-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang][ARM] Include arm_acle.h in intrin.h on supported platforms #144172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-backend-arm @llvm/pr-subscribers-backend-x86 Author: Nick Sarnie (sarnex) ChangesWarnings should suggest changes that are valid on both MSVC and Clang where possible. See #140910 for more info. Full diff: https://github.com/llvm/llvm-project/pull/144172.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/BuiltinHeaders.def b/clang/include/clang/Basic/BuiltinHeaders.def
index 22668ec7a3396..8e4a2f9bee9aa 100644
--- a/clang/include/clang/Basic/BuiltinHeaders.def
+++ b/clang/include/clang/Basic/BuiltinHeaders.def
@@ -12,7 +12,6 @@
//===----------------------------------------------------------------------===//
HEADER(NO_HEADER, nullptr)
-HEADER(ARMACLE_H, "arm_acle.h")
HEADER(BLOCKS_H, "Blocks.h")
HEADER(COMPLEX_H, "complex.h")
HEADER(CTYPE_H, "ctype.h")
diff --git a/clang/include/clang/Basic/BuiltinsAArch64.def b/clang/include/clang/Basic/BuiltinsAArch64.def
index 8867a9fe09fb9..4a1db34b9b30f 100644
--- a/clang/include/clang/Basic/BuiltinsAArch64.def
+++ b/clang/include/clang/Basic/BuiltinsAArch64.def
@@ -50,11 +50,11 @@ BUILTIN(__builtin_arm_wfi, "v", "")
BUILTIN(__builtin_arm_sev, "v", "")
BUILTIN(__builtin_arm_sevl, "v", "")
BUILTIN(__builtin_arm_chkfeat, "WUiWUi", "")
-TARGET_HEADER_BUILTIN(__yield, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__wfe, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__wfi, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__sev, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__sevl, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__yield, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__wfe, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__wfi, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__sev, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__sevl, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
// Like __builtin_trap but provide an 16-bit immediate reason code (which goes into `brk #N`).
BUILTIN(__builtin_arm_trap, "vUIs", "nr")
@@ -87,9 +87,9 @@ TARGET_BUILTIN(__builtin_arm_mops_memset_tag, "v*v*iz", "", "mte,mops")
BUILTIN(__builtin_arm_dmb, "vUi", "nc")
BUILTIN(__builtin_arm_dsb, "vUi", "nc")
BUILTIN(__builtin_arm_isb, "vUi", "nc")
-TARGET_HEADER_BUILTIN(__dmb, "vUi", "nch", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__dsb, "vUi", "nch", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__isb, "vUi", "nch", ARMACLE_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__dmb, "vUi", "nch", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__dsb, "vUi", "nch", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__isb, "vUi", "nch", INTRIN_H, ALL_LANGUAGES, "")
TARGET_BUILTIN(__builtin_arm_jcvt, "Zid", "nc", "v8.3a")
diff --git a/clang/include/clang/Basic/BuiltinsARM.def b/clang/include/clang/Basic/BuiltinsARM.def
index 2592e25e95c37..20a41dc248859 100644
--- a/clang/include/clang/Basic/BuiltinsARM.def
+++ b/clang/include/clang/Basic/BuiltinsARM.def
@@ -186,19 +186,19 @@ BUILTIN(__builtin_arm_wfi, "v", "")
BUILTIN(__builtin_arm_sev, "v", "")
BUILTIN(__builtin_arm_sevl, "v", "")
BUILTIN(__builtin_arm_dbg, "vUi", "")
-TARGET_HEADER_BUILTIN(__yield, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__wfe, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__wfi, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__sev, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__sevl, "v", "h", ARMACLE_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__yield, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__wfe, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__wfi, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__sev, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__sevl, "v", "h", INTRIN_H, ALL_LANGUAGES, "")
// Data barrier
BUILTIN(__builtin_arm_dmb, "vUi", "nc")
BUILTIN(__builtin_arm_dsb, "vUi", "nc")
BUILTIN(__builtin_arm_isb, "vUi", "nc")
-TARGET_HEADER_BUILTIN(__dmb, "vUi", "nch", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__dsb, "vUi", "nch", ARMACLE_H, ALL_LANGUAGES, "")
-TARGET_HEADER_BUILTIN(__isb, "vUi", "nch", ARMACLE_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__dmb, "vUi", "nch", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__dsb, "vUi", "nch", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__isb, "vUi", "nch", INTRIN_H, ALL_LANGUAGES, "")
// Prefetch
BUILTIN(__builtin_arm_prefetch, "vvC*UiUi", "nc")
diff --git a/clang/lib/Headers/intrin.h b/clang/lib/Headers/intrin.h
index 3dd1eb45817d4..969a5bb4aa81f 100644
--- a/clang/lib/Headers/intrin.h
+++ b/clang/lib/Headers/intrin.h
@@ -30,6 +30,10 @@
#include <arm64intr.h>
#endif
+#if defined(__ARM_ACLE)
+#include <arm_acle.h>
+#endif
+
/* For the definition of jmp_buf. */
#if __STDC_HOSTED__
#include <setjmp.h>
diff --git a/clang/test/CodeGen/arm-former-microsoft-intrinsics-header-warning.c b/clang/test/CodeGen/arm-former-microsoft-intrinsics-header-warning.c
index 8edcbbeb0375d..19216629373ef 100644
--- a/clang/test/CodeGen/arm-former-microsoft-intrinsics-header-warning.c
+++ b/clang/test/CodeGen/arm-former-microsoft-intrinsics-header-warning.c
@@ -3,48 +3,48 @@
void check__dmb(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__dmb(0);
}
void check__dsb(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__dsb(0);
}
void check__isb(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__isb(0);
}
void check__yield(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__yield();
}
void check__wfe(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__wfe();
}
void check__wfi(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__wfi();
}
void check__sev(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__sev();
}
void check__sevl(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__sevl();
}
diff --git a/clang/test/CodeGen/arm64-former-microsoft-intrinsics-header-warning.c b/clang/test/CodeGen/arm64-former-microsoft-intrinsics-header-warning.c
index 52fed49db4dd2..3f829d39b413b 100644
--- a/clang/test/CodeGen/arm64-former-microsoft-intrinsics-header-warning.c
+++ b/clang/test/CodeGen/arm64-former-microsoft-intrinsics-header-warning.c
@@ -6,48 +6,48 @@
void check__dmb(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__dmb(0);
}
void check__dsb(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__dsb(0);
}
void check__isb(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__isb(0);
}
void check__yield(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__yield();
}
void check__wfe(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__wfe();
}
void check__wfi(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__wfi();
}
void check__sev(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__sev();
}
void check__sevl(void) {
// expected-warning@+2{{call to undeclared library function}}
- // expected-note@+1{{include the header <arm_acle.h> or explicitly provide a declaration for}}
+ // expected-note@+1{{include the header <intrin.h> or explicitly provide a declaration for}}
__sevl();
}
diff --git a/clang/test/Headers/arm-acle-no-direct-include.c b/clang/test/Headers/arm-acle-no-direct-include.c
new file mode 100644
index 0000000000000..26e7c914ce833
--- /dev/null
+++ b/clang/test/Headers/arm-acle-no-direct-include.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cl --target=aarch64-windows-msvc -Xclang -verify /E -U__STDC_HOSTED__ -Wno-builtin-macro-redefined %s 2>&1 | FileCheck %s
+
+// expected-no-diagnostics
+
+// CHECK: void __yield(void);
+#include <intrin.h>
+void f() { __yield(); }
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intrin.h only exists on Windows; what's supposed to happen on non-Windows targets? (These functions are defined in the ACLE: see https://arm-software.github.io/acle/main/acle.html .)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I missed that. I guess the best solution is to have a different warning for Linux/Windows.
@rnk Opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnk Ping on this one, thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnk Ping x2 when you get a chance, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnk Ping x3 on this one,
Forgot about this for a sec and 5 months passed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a different warning on different targets seems fine with me. I don't think we have the infrastructure for that, though.
If you want to land the change to intrin.h separately, I'd be fine with that. (Including arm_acle.h with clang-cl should work; it just won't work with MSVC.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I just changed this PR to only include the intrin.h
change, please take a look when you get a sec
Signed-off-by: Sarnie, Nick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good to me.
Sorry for the delay, I've had various conferences. In general, it makes me think we need to grow more maintainers and delegate more approval power. That's clearly a long-term project, but any one maintainer can only be so available. :)
No problem, thanks for the review! |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/31122 Here is the relevant piece of the build log for the reference
|
Hi @sarnex, not sure if you were notified, but the test you added is failing on MacOS: https://lab.llvm.org/buildbot/#/builders/190/builds/27443
Should be a simple fix I think, but can you take a look? |
Ah sorry, investigating now. |
…158677) See [here](#144172 (comment)), need to add `--` to filename parsing on Mac because the path starts with `/U` which `clang-cl` treats as the flag `/U`. Signed-off-by: Sarnie, Nick <[email protected]>
…c on Mac (#158677) See [here](llvm/llvm-project#144172 (comment)), need to add `--` to filename parsing on Mac because the path starts with `/U` which `clang-cl` treats as the flag `/U`. Signed-off-by: Sarnie, Nick <[email protected]>
Right now when using ARM intrinsics without including
arm_acle.h
, we throw a warning saying to include it or provide a declaration for the function.MSVC doesn't have any ARM-intrinsic specific header, so include Clang's ARM intrinsic header (
arm_acle.h
) inintrin.h
so we don't get a warning that tells the user to include a header that doesn't exist. Ideally we could change the header based on the platform, but we don't have the infra for that at the moment.See #140910 for more info.