[clang][codegen] Don't mark "int" TBAA on FP libcalls with indirect args #108853
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Benjamin Maxwell (MacDue) ChangesOn some targets, an FP libcall with argument types such as long double Currently, this can be seen for long doubles on x86_64-w64-mingw32. The Nothing seems to break due to this currently as the metadata is being Full diff: https://github.com/llvm/llvm-project/pull/108853.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 27abeba92999b3..5730e7867a648f 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -690,23 +690,37 @@ 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));
+ llvm::CallBase *callOrInvoke = nullptr;
+ CGFunctionInfo const *FnInfo = nullptr;
RValue Call =
- CGF.EmitCall(E->getCallee()->getType(), callee, E, ReturnValueSlot());
+ CGF.EmitCall(E->getCallee()->getType(), callee, E, ReturnValueSlot(),
+ /*Chain=*/nullptr, &callOrInvoke, &FnInfo);
if (unsigned BuiltinID = FD->getBuiltinID()) {
// Check whether a FP math builtin function, such as BI__builtin_expf
ASTContext &Context = CGF.getContext();
bool ConstWithoutErrnoAndExceptions =
Context.BuiltinInfo.isConstWithoutErrnoAndExceptions(BuiltinID);
+
+ // Before annotating this libcall with "int" TBAA metadata check all
+ // arguments are passed directly. On some targets, types such as "long
+ // double" are passed indirectly via a pointer, and annotating the call with
+ // "int" TBAA metadata will lead to set up for those arguments being
+ // incorrectly optimized out.
+ bool AllArgumentsPassedDirectly =
+ llvm::all_of(FnInfo->arguments(), [&](auto const &ArgInfo) {
+ return ArgInfo.info.isDirect() || ArgInfo.info.isExtend();
+ });
+
// Restrict to target with errno, for example, MacOS doesn't set errno.
// TODO: Support builtin function with complex type returned, eg: cacosh
- if (ConstWithoutErrnoAndExceptions && CGF.CGM.getLangOpts().MathErrno &&
- !CGF.Builder.getIsFPConstrained() && Call.isScalar()) {
+ if (AllArgumentsPassedDirectly && ConstWithoutErrnoAndExceptions &&
+ CGF.CGM.getLangOpts().MathErrno && !CGF.Builder.getIsFPConstrained() &&
+ Call.isScalar()) {
// Emit "int" TBAA metadata on FP math libcalls.
clang::QualType IntTy = Context.IntTy;
TBAAAccessInfo TBAAInfo = CGF.CGM.getTBAAAccessInfo(IntTy);
- Instruction *Inst = cast<llvm::Instruction>(Call.getScalarVal());
- CGF.CGM.DecorateInstructionWithTBAA(Inst, TBAAInfo);
+ CGF.CGM.DecorateInstructionWithTBAA(callOrInvoke, TBAAInfo);
}
}
return Call;
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 35b5daaf6d4b55..9166db4c74128c 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -5932,7 +5932,8 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
const CGCallee &OrigCallee, const CallExpr *E,
ReturnValueSlot ReturnValue,
llvm::Value *Chain,
- llvm::CallBase **CallOrInvoke) {
+ llvm::CallBase **CallOrInvoke,
+ CGFunctionInfo const **ResolvedFnInfo) {
// Get the actual function type. The callee type will always be a pointer to
// function type or a block pointer type.
assert(CalleeType->isFunctionPointerType() &&
@@ -6111,6 +6112,9 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
const CGFunctionInfo &FnInfo = CGM.getTypes().arrangeFreeFunctionCall(
Args, FnType, /*ChainCall=*/Chain);
+ if (ResolvedFnInfo)
+ *ResolvedFnInfo = &FnInfo;
+
// C99 6.5.2.2p6:
// If the expression that denotes the called function has a type
// that does not include a prototype, [the default argument
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 9f868c98458996..1755dab3e3e4e8 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -4396,7 +4396,8 @@ class CodeGenFunction : public CodeGenTypeCache {
}
RValue EmitCall(QualType FnType, const CGCallee &Callee, const CallExpr *E,
ReturnValueSlot ReturnValue, llvm::Value *Chain = nullptr,
- llvm::CallBase **CallOrInvoke = nullptr);
+ llvm::CallBase **CallOrInvoke = nullptr,
+ CGFunctionInfo const **ResolvedFnInfo = nullptr);
// If a Call or Invoke instruction was emitted for this CallExpr, this method
// writes the pointer to `CallOrInvoke` if it's not null.
diff --git a/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c b/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
new file mode 100644
index 00000000000000..56c6b3ec00bc7e
--- /dev/null
+++ b/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
@@ -0,0 +1,36 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang_cc1 -triple=x86_64-w64-mingw32 -fmath-errno -O3 -emit-llvm -o - %s | FileCheck %s -check-prefixes=CHECK
+
+long double powl(long double a, long double b);
+
+// Negative test: powl is a floating-point math function that is
+// ConstWithoutErrnoAndExceptions, however, for this target long doubles are
+// passed indirectly via a pointer. Annotating the call with "int" TBAA metadata
+// will cause the setup for the BYVAL arguments to be incorrectly optimized out.
+
+// CHECK-LABEL: define dso_local void @test_powl(
+// CHECK-SAME: ptr dead_on_unwind noalias nocapture writable writeonly sret(x86_fp80) align 16 [[AGG_RESULT:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: [[TMP:%.*]] = alloca x86_fp80, align 16
+// CHECK-NEXT: [[BYVAL_TEMP:%.*]] = alloca x86_fp80, align 16
+// CHECK-NEXT: [[BYVAL_TEMP1:%.*]] = alloca x86_fp80, align 16
+// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 16, ptr nonnull [[BYVAL_TEMP]]) #[[ATTR3:[0-9]+]]
+// CHECK-NEXT: store x86_fp80 0xK40008000000000000000, ptr [[BYVAL_TEMP]], align 16, !tbaa [[TBAA3:![0-9]+]]
+// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 16, ptr nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
+// CHECK-NEXT: store x86_fp80 0xK40008000000000000000, ptr [[BYVAL_TEMP1]], align 16, !tbaa [[TBAA3]]
+// CHECK-NEXT: call void @powl(ptr dead_on_unwind nonnull writable sret(x86_fp80) align 16 [[TMP]], ptr noundef nonnull [[BYVAL_TEMP]], ptr noundef nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
+// CHECK-NEXT: [[TMP0:%.*]] = load x86_fp80, ptr [[TMP]], align 16, !tbaa [[TBAA3]]
+// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 16, ptr nonnull [[BYVAL_TEMP]]) #[[ATTR3]]
+// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 16, ptr nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
+// CHECK-NEXT: store x86_fp80 [[TMP0]], ptr [[AGG_RESULT]], align 16, !tbaa [[TBAA3]]
+// CHECK-NEXT: ret void
+//
+long double test_powl() {
+ return powl(2.0L, 2.0L); // Don't emit TBAA metadata
+}
+//.
+// CHECK: [[TBAA3]] = !{[[META4:![0-9]+]], [[META4]], i64 0}
+// CHECK: [[META4]] = !{!"long double", [[META5:![0-9]+]], i64 0}
+// CHECK: [[META5]] = !{!"omnipotent char", [[META6:![0-9]+]], i64 0}
+// CHECK: [[META6]] = !{!"Simple C/C++ TBAA"}
+//.
|
|
Note the first commit (6db9f6d) shows a correctness issue with what's currently upstream as the |
|
Thanks for the fix! Unfortunately I'm way out of my depth when it comes to actually reviewing this change, so hopefully you can find someone else to check/review it - but I appreciate you finding the root cause so quickly! |
|
How does this interact with #107598? |
I think it's solving the same problem, but a different way (looking at the final LLVM function type rather than checking the ABI information). Though this also changes things to ensure when TBAA data is set, it's always set on the call. |
Wasn't already doing that? (setting the TBAA on the call?). |
It was setting it on |
Without this patch and without (#107598) the function
|
I'm not sure I follow, but the issue I spotted was "int" TBAA metadata was being set on the load following the See the diff from the first commit: diff --git a/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c b/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
index dd013dcc8b3ca8..56c6b3ec00bc7e 100644
--- a/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
+++ b/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
@@ -19,7 +19,7 @@ long double powl(long double a, long double b);
// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 16, ptr nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
// CHECK-NEXT: store x86_fp80 0xK40008000000000000000, ptr [[BYVAL_TEMP1]], align 16, !tbaa [[TBAA3]]
// CHECK-NEXT: call void @powl(ptr dead_on_unwind nonnull writable sret(x86_fp80) align 16 [[TMP]], ptr noundef nonnull [[BYVAL_TEMP]], ptr noundef nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
-// CHECK-NEXT: [[TMP0:%.*]] = load x86_fp80, ptr [[TMP]], align 16, !tbaa [[TBAA7:![0-9]+]]
+// CHECK-NEXT: [[TMP0:%.*]] = load x86_fp80, ptr [[TMP]], align 16, !tbaa [[TBAA3]]
// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 16, ptr nonnull [[BYVAL_TEMP]]) #[[ATTR3]]
// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 16, ptr nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
// CHECK-NEXT: store x86_fp80 [[TMP0]], ptr [[AGG_RESULT]], align 16, !tbaa [[TBAA3]]
@@ -33,6 +33,4 @@ long double test_powl() {
// CHECK: [[META4]] = !{!"long double", [[META5:![0-9]+]], i64 0}
// CHECK: [[META5]] = !{!"omnipotent char", [[META6:![0-9]+]], i64 0}
// CHECK: [[META6]] = !{!"Simple C/C++ TBAA"}
-// CHECK: [[TBAA7]] = !{[[META8:![0-9]+]], [[META8]], i64 0}
-// CHECK: [[META8]] = !{!"int", [[META5]], i64 0}
//.
That looks okay though? It's not passing or returning values via pointers, so it should be safe to set the "int" TBAA (which indicates the only pointer it could read is errno). |
Not really! |
I believe the intention of both #96025 and #100302 was to set the metadata on the call. The case I mentioned where it was attached to the load after the call just seemed like an unintended consequence of attaching the metadata to the result value. It sounds like both those changes break your downstream compiler? My understanding is "int" TBAA metadata is set on floating-point math calls (that don't take pointers) to indicate the only memory they could read/write to is |
Oh wait, then you say "not really" you're only referring to the case where the arguments are passed indirectly (i.e via pointers) not the case where they're passed via floating-point registers. This patch is updating the check to look at the ABI information and prevents setting the "int" TBAA metadata on calls that pass values via pointers. |
Yes exactly! Have you tried running the test case in patch #107598 to see what IR it generates? |
I've updated my test (based on yours), and I think you can now see the TBAA "int" metadata is only set on the call when the arguments are not being passed via pointers. |
MacDue
force-pushed
the
fix_tbaa_2
branch
2 times, most recently
from
c52d6c9 to
f5d293c
Compare
|
OK. This LGTM. I guess we can adopt this PR instead of #107598 unless other reviewers have more to say. |
|
I'll land this later today if there's no further comments 🙂 |
On some targets, an FP libcall with argument types such as long double will be lowered to pass arguments indirectly via pointers. When this is the case we should not mark the libcall with "int" TBAA as it may lead to incorrect optimizations. Currently, this can be seen for long doubles on x86_64-w64-mingw32. The `load x86_fp80` after the call is (incorrectly) marked with "int" TBAA (overwriting the previous metadata for "long double"). Nothing seems to break due to this currently as the metadata is being incorrectly placed on the load and not the call. But if the metadata is moved to the call (which this patch ensures), LLVM will optimize out the setup for the arguments.
I've added a test case for |
7 participants
On some targets, an FP libcall with argument types such as long double
will be lowered to pass arguments indirectly via pointers. When this is
the case we should not mark the libcall with "int" TBAA as it may lead
to incorrect optimizations.
Currently, this can be seen for long doubles on x86_64-w64-mingw32. The
load x86_fp80after the call is (incorrectly) marked with "int" TBAA(overwriting the previous metadata for "long double").
Nothing seems to break due to this currently as the metadata is being
incorrectly placed on the load and not the call. But if the metadata
is moved to the call (which this patch ensures), LLVM will optimize out
the setup for the arguments.