-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang codegen] Emit int TBAA metadata on FP math libcall expf #96025
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-clang-codegen @llvm/pr-subscribers-clang Author: Allen (vfdff) ChangesBase on the discussion https://discourse.llvm.org/t/fp-can-we-add-pure-attribute-for-math-library-functions-default/79459, math libcalls set errno, so it should emit "int" TBAA metadata on FP libcalls to solve the alias issue. Fix #86635 Full diff: https://github.com/llvm/llvm-project/pull/96025.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 08a89bd123d03..dc4af109cdbdb 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -707,7 +707,38 @@ static RValue emitLibraryCall(CodeGenFunction &CGF, const FunctionDecl *FD,
const CallExpr *E, llvm::Constant *calleeValue) {
CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
CGCallee callee = CGCallee::forDirect(calleeValue, GlobalDecl(FD));
- return CGF.EmitCall(E->getCallee()->getType(), callee, E, ReturnValueSlot());
+ RValue Call =
+ CGF.EmitCall(E->getCallee()->getType(), callee, E, ReturnValueSlot());
+
+ // Check the supported intrinsic.
+ if (unsigned BuiltinID = FD->getBuiltinID()) {
+ auto IntrinsicID = [&]() -> unsigned {
+ switch (BuiltinID) {
+ case Builtin::BIexpf:
+ case Builtin::BI__builtin_expf:
+ case Builtin::BI__builtin_expf128:
+ return true;
+ }
+ // TODO: support more FP math libcalls
+ return false;
+ }();
+
+ if (IntrinsicID) {
+ llvm::MDBuilder MDHelper(CGF.getLLVMContext());
+ MDNode *RootMD;
+ if (CGF.getLangOpts().CPlusPlus)
+ RootMD = MDHelper.createTBAARoot("Simple C++ TBAA");
+ else
+ RootMD = MDHelper.createTBAARoot("Simple C/C++ TBAA");
+ // Emit "int" TBAA metadata on FP math libcalls.
+ MDNode *AliasType = MDHelper.createTBAANode("int", RootMD);
+ MDNode *MDInt = MDHelper.createTBAAStructTagNode(AliasType, AliasType, 0);
+
+ Value *Val = Call.getScalarVal();
+ cast<llvm::Instruction>(Val)->setMetadata(LLVMContext::MD_tbaa, MDInt);
+ }
+ }
+ return Call;
}
/// Emit a call to llvm.{sadd,uadd,ssub,usub,smul,umul}.with.overflow.*
diff --git a/clang/test/CodeGen/math-libcalls-tbaa.cpp b/clang/test/CodeGen/math-libcalls-tbaa.cpp
new file mode 100644
index 0000000000000..b3241c97da71f
--- /dev/null
+++ b/clang/test/CodeGen/math-libcalls-tbaa.cpp
@@ -0,0 +1,22 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
+// RUN: %clang -S -O3 -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK
+
+#include <math.h>
+
+
+// Emit int TBAA metadata on FP math libcalls, which is useful for alias analysis
+
+// CHECK-LABEL: define dso_local noundef float @_Z3fooPffi
+// CHECK-SAME: (ptr nocapture noundef readonly [[NUM:%.*]], float noundef [[R2INV:%.*]], i32 noundef [[N:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: entry:
+// CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds i8, ptr [[NUM]], i64 40
+// CHECK-NEXT: [[TMP0:%.*]] = load float, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA6:![0-9]+]]
+// CHECK-NEXT: [[CALL_I:%.*]] = tail call noundef float @expf(float noundef [[TMP0]]) #[[ATTR2:[0-9]+]], !tbaa [[TBAA10:![0-9]+]]
+// CHECK-NEXT: [[MUL:%.*]] = fmul float [[TMP0]], [[CALL_I]]
+// CHECK-NEXT: ret float [[MUL]]
+//
+float foo (float num[], float r2inv, int n) {
+ const float expm2 = std::exp(num[10]); # Emit TBAA metadata on @expf
+ float tmp = expm2 * num[10];
+ return tmp;
+}
|
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.
Ideally we could identify errno more precisely somehow, but I guess this is better than nothing.
Can we restrict this to targets where libm actually modifies errno?
How does this interact with StrictFP?
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
llvm::MDBuilder MDHelper(CGF.getLLVMContext()); | ||
MDNode *RootMD; | ||
if (CGF.getLangOpts().CPlusPlus) | ||
RootMD = MDHelper.createTBAARoot("Simple C++ TBAA"); |
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.
Please don't copy-paste code from CodeGenTBAA.
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.
I don't find the way to call CodeGenTBAA::getRoot directly, so add a public interface getTBAARoot, thanks.
Do you mean it is better to have a function list, which actually modifies
I don't think it related to StrictFP, because we only care whether the global variable |
hi @efriedma-quic, I surprise to find there are 4 new failed tests with new commit 9990877a2 because timeout, but the change seems tiny, and doesn't see marked change on compile time local with this commit , so I need some time to figure out the reason. If you have any suggestions, it's also very welcome. |
I said targets, not functions. For example, MacOS never set errno for math functions. |
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
llvm::MDBuilder MDHelper(CGF.getLLVMContext()); | ||
MDNode *RootMD = CGF.CGM.getTBAARoot(); | ||
// Emit "int" TBAA metadata on FP math libcalls. | ||
MDNode *AliasType = MDHelper.createTBAANode("int", RootMD); |
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.
We should have code somewhere for getting metadata for "int"?
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, I don't find the metadata for "int", and the createTBAAScalarTypeNode is only used in CodeGenTBAA::createScalarTypeNode
ps:I will update the createTBAANode into createTBAAScalarTypeNode according switch from old TBAA format to the new struct-path aware TBAA format
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.
Looked it up; should be CGM.getTBAATypeInfo(Context.IntTy)
?
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.
Yes, apply your comment, thanks
I can't think of any obvious reason this would cause timeouts. I mean, you're adding metadata to a bunch of functions, and if something handles that metadata inefficiently, things could easily explode. But nothing specific comes to mind. |
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
}(); | ||
|
||
const llvm::Triple &T = CGF.getTarget().getTriple(); | ||
// Restrict to Linux because not all targets set errno, such as MacOS. |
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.
LangOpts.MathErrno
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
llvm::MDBuilder MDHelper(CGF.getLLVMContext()); | ||
MDNode *RootMD = CGF.CGM.getTBAARoot(); | ||
// Emit "int" TBAA metadata on FP math libcalls. | ||
MDNode *AliasType = MDHelper.createTBAANode("int", RootMD); |
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.
Looked it up; should be CGM.getTBAATypeInfo(Context.IntTy)
?
It report error Access tag metadata must have either 4 or 5 operands with CGM.getTBAATypeInfo(Context.IntTy)
|
Oh, it works fine when I delete the option -Xclang -new-struct-path-tbaa |
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
// Emit "int" TBAA metadata on FP math libcalls. | ||
clang::QualType IntTy = Context.IntTy; | ||
MDNode *AliasType = CGF.CGM.getTBAATypeInfo(IntTy); | ||
MDNode *MDInt = MDHelper.createTBAAStructTagNode(AliasType, AliasType, 0); |
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.
I think you want CodeGenModule::getTBAAAccessTagInfo, not getTBAATypeInfo? (My mistake.)
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.
Apply your comment, thanks very much
hi, is there any new suggestion ? |
ping ? |
|
||
// RUN: %clang -S -O3 -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,NoNewStructPathTBAA | ||
// RUN: %clang -S -O3 -Xclang -new-struct-path-tbaa -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,NewStructPathTBAA | ||
|
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.
Why are you using clang here instead of clang_cc1. I think in general CodeGen tests should use clang_cc1. I would think the same than what the other tbaa LIT tests are written.
Why does the test fail on Windows? I don't see any reason to require Linux for this test.
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.
- I add a head file <math.h> in the test, and it could not find the head file with clang_cc1
I also tried %clang_cc1 -triple aarch64 -internal-isystem %S/../Headers/Inputs/include, but it did not run into the function emitLibraryCall, which is changed by this PR
- I'll try to relax the restriction, add that for windows because it run timeout with my first commit.
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.
hi @zahiraam, It still fail wtih run timeout for window, so would it be better to add this restriction?
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.
Can you rewrite the test so you don't need to include math.h? You don't need the whole header; you just need extern "C" float expf(float);
.
We try to avoid including system headers in clang regression tests so the tests work consistently across platforms. If we actually need to run something natively for some reason, we it in llvm-test-suite.
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, apply your idea. But the test is still fail with timeout on the windows target.
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.
@vfdff I have uploaded your patch and ran the LIT test on windows. It's failing mostly because the //CHECK lines you have in your LIT test are Linux's output. What you are interested by is the tbaa metada at the call of expf
right? So it's enough to just put that check line.
This test is passing for me on Windows (I didn't test it on Linux. Can you do that?):
// RUN: %clang_cc1 -fmath-errno -O3 -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,NoNewStructPathTBAA
// RUN: %clang_cc1 -fmath-errno -O3 -new-struct-path-tbaa -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,NewStructPathTBAA
extern "C" float expf(float);
// Emit int TBAA metadata on FP math libcalls, which is useful for alias analysis
// CHECK-LABEL: define dso_local noundef float {{.*}}foo
// CHECK-SAME: ptr nocapture noundef readonly [[NUM:%.*]], float noundef [[R2INV:%.*]], i32 noundef [[N:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
// CHECK-NEXT: [[ENTRY:.*:]]
// CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds i8, ptr [[NUM]], i64 40
// CHECK-NEXT: [[TMP0:%.*]] = load float, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA2:![0-9]+]]
// CHECK-NEXT: [[CALL:%.*]] = tail call float @expf(float noundef [[TMP0]]) #[[ATTR2:[0-9]+]]
float foo (float num[], float r2inv, int n) {
const float expm2 = expf(num[10]); // Emit TBAA metadata on @expf
float tmp = expm2 * num[10];
return tmp;
}
//.
// NoNewStructPathTBAA: [[TBAA2]] = !{[[META3:![0-9]+]], [[META3]], i64 0}
// NoNewStructPathTBAA: [[META3]] = !{!"float", [[META4:![0-9]+]], i64 0}
// NoNewStructPathTBAA: [[META4]] = !{!"omnipotent char", [[META5:![0-9]+]], i64 0}
// NoNewStructPathTBAA: [[META5]] = !{!"Simple C++ TBAA"}
//.
// NewStructPathTBAA: [[TBAA2]] = !{[[META3:![0-9]+]], [[META3]], i64 0, i64 4}
// NewStructPathTBAA: [[META3]] = !{[[META4:![0-9]+]], i64 4, !"float"}
// NewStructPathTBAA: [[META4]] = !{[[META5:![0-9]+]], i64 1, !"omnipotent char"}
// NewStructPathTBAA: [[META5]] = !{!"Simple C++ TBAA"}
//.
//// NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
// NewStructPathTBAA: {{.*}}
// NoNewStructPathTBAA: {{.*}}
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.
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.
As there is difference function naming rules between Linux and Windows, I try to add extern "C"
for the function, then the case pass on windows now, see #98704. I'll rebase this PR after PR98704 is accepted.
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.
Yes, you are right I didn't notice that. Sorry. There is no TBAA on the expf
call. But there should be since I uploaded your entire patch! I don't understand why the extern C
would make any difference. Furthermore, I ran the LIT test with it.
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 for checking.
I think extern C will make the output function names in the assembly named after the standard C language, i.e. no extra prefix and suffix.
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.
How does this interact with StrictFP?
I don't think it related to StrictFP, because we only care whether the global variable errno is modified? which may bring in alias to other global variable. As now the type of errno is int, so we record that for related function.
In strictfp mode, expf() can also modify the floating-point status bits... which count as "memory". So we need to make sure we treat reads and writes to the status bits as aliasing.
|
||
// RUN: %clang -S -O3 -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,NoNewStructPathTBAA | ||
// RUN: %clang -S -O3 -Xclang -new-struct-path-tbaa -emit-llvm -o - -x c++ %s | FileCheck %s -check-prefixes=CHECK,NewStructPathTBAA | ||
|
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.
There isn't any obvious reason for it to time out on Windows specifically... does specifying a target triple help? Otherwise, not sure. We definitely need to sort out whatever is happening if the compiler breaks on Windows.
hi @efriedma-quic I find the case will timeout even without this changes ,so would it reasonable to add require Linux for this test now on PR98704 ?
|
Base on the discussion https://discourse.llvm.org/t/fp-can-we-add-pure-attribute-for-math-library-functions-default/79459, math libcalls set errno, so it should emit "int" TBAA metadata on FP libcalls to solve the alias issue. Fix llvm#86635
hi @efriedma-quic @zahiraam I have rebase the PR base on the top main branch, and also addresss all comments, please help to check any new sugguestion? thanks. |
LGTM. 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.
LGTM with one minor comment
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
|
||
// Check the supported intrinsic. | ||
if (unsigned BuiltinID = FD->getBuiltinID()) { | ||
auto IntrinsicID = [&]() -> unsigned { |
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.
"IntrinsicID" is a boolean; please rename (maybe something like IsErrnoIntrinsic
).
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.
apply your comment, thanks
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/1154 Here is the relevant piece of the build log for the reference:
|
Summary: Base on the discussion https://discourse.llvm.org/t/fp-can-we-add-pure-attribute-for-math-library-functions-default/79459, math libcalls set errno, so it should emit "int" TBAA metadata on FP libcalls to solve the alias issue. Note: Only add support for expf in this PR Fix #86635 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251692
Base on the discussion https://discourse.llvm.org/t/fp-can-we-add-pure-attribute-for-math-library-functions-default/79459, math libcalls set errno, so it should emit "int" TBAA metadata on FP libcalls to solve the alias issue.
Note: Only add support for expf in this PR
Fix #86635