[clang codegen] Emit int TBAA metadata on more FP math libcalls #100302

vfdff · 2024-07-24T05:13:59Z

Follow #96025, except expf, more FP math libcalls in libm should also be supported.

Fix #86635

Follow PR96025, except expf, more FP math libcalls in libm should also be supported. Fix llvm#86635

llvmbot · 2024-07-24T05:14:35Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Allen (vfdff)

Changes

Follow PR96025, except expf, more FP math libcalls in libm should also be supported.

Fix #86635

Full diff: https://github.com/llvm/llvm-project/pull/100302.diff

1 Files Affected:

(modified) clang/lib/CodeGen/CGBuiltin.cpp (+7-8)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index a0d03b87ccdc9..a9696ebe61e3a 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -692,23 +692,22 @@ static RValue emitLibraryCall(CodeGenFunction &CGF, const FunctionDecl *FD,
   RValue Call =
       CGF.EmitCall(E->getCallee()->getType(), callee, E, ReturnValueSlot());
 
-  // Check the supported intrinsic.
+  ASTContext &Context = CGF.getContext();
   if (unsigned BuiltinID = FD->getBuiltinID()) {
     auto IsErrnoIntrinsic = [&]() -> unsigned {
-      switch (BuiltinID) {
-      case Builtin::BIexpf:
-      case Builtin::BI__builtin_expf:
-      case Builtin::BI__builtin_expf128:
+      // Check whether a FP math builtin function, such as BI__builtin_expf
+      QualType ResultTy = FD->getReturnType();
+      bool IsMathLibCall =
+          Context.BuiltinInfo.isLibFunction(BuiltinID) ||
+          Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID);
+      if (IsMathLibCall && CGF.ConvertType(ResultTy)->isFloatingPointTy())
         return true;
-      }
-      // TODO: support more FP math libcalls
       return false;
     }();
 
     // Restrict to target with errno, for example, MacOS doesn't set errno.
     if (IsErrnoIntrinsic && CGF.CGM.getLangOpts().MathErrno &&
         !CGF.Builder.getIsFPConstrained()) {
-      ASTContext &Context = CGF.getContext();
       // Emit "int" TBAA metadata on FP math libcalls.
       clang::QualType IntTy = Context.IntTy;
       TBAAAccessInfo TBAAInfo = CGF.CGM.getTBAAAccessInfo(IntTy);

arsenm

Needs tests

vfdff · 2024-07-24T09:52:45Z

Needs tests

Add a new test foo_fabs, and the prior test foo is also works, thanks

arsenm · 2024-07-24T11:11:08Z

clang/lib/CodeGen/CGBuiltin.cpp

-  // Check the supported intrinsic.
+  ASTContext &Context = CGF.getContext();
  if (unsigned BuiltinID = FD->getBuiltinID()) {
    auto IsErrnoIntrinsic = [&]() -> unsigned {


The name here IsErrnoIntrinsic is incorrect for fabs which cannot set errno

Oh, your are right, so maybe list them one by one ?

arsenm · 2024-07-24T11:11:34Z

clang/lib/CodeGen/CGBuiltin.cpp

+      bool IsMathLibCall =
+          Context.BuiltinInfo.isLibFunction(BuiltinID) ||
+          Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID);
+      if (IsMathLibCall && CGF.ConvertType(ResultTy)->isFloatingPointTy())


These conditions do not imply it sets errno. Also this can be return of boolean expression

My idea was to get the floating-point interface from libc/libm, so I has extra constraints **CGF.ConvertType(ResultTy)->isFloatingPointTy() guarding the return type" and CGF.CGM.getLangOpts().MathErrno guarding the errno in the following line 709.

The errno-ness is already a builtin property you should be able to query from BuiltinInfo

Although in the context of this patch, I don't see why any of this is relevant for emitting TBAA

For the builtin which doesn't set errno is sure not need add extra TBAA, the origin intend is address these builtins may set errno, such as expf, so we emit "int" TBAA metadata on them will make sure it is safe to reuse the float value from memory.

So check isConstWithoutErrnoAndExceptions?

No , we expect it works without explicit disable the MathErrno, just similar to the behavior of gcc, https://gcc.godbolt.org/z/rsbqsGWMv. it is discussed on discourse

Oh, you are right, I see your idea, the expect list can be get by isConstWithoutErrnoAndExceptions, thanks

arsenm · 2024-07-28T08:00:47Z

clang/lib/CodeGen/CGBuiltin.cpp

+      bool ConstWithoutErrnoAndExceptions =
+          Context.BuiltinInfo.isConstWithoutErrnoAndExceptions(BuiltinID);
+      if (ConstWithoutErrnoAndExceptions &&
+          CGF.ConvertType(ResultTy)->isFloatingPointTy())


The isFloatingPointTy check should be unnecessary. You can then just remove the whole lambda and directly check isConstWithoutErrnoAndExceptions

arsenm · 2024-07-28T08:01:01Z

clang/test/CodeGen/math-libcalls-tbaa.cpp

+   double tmp = expm2 * num[10];
+   return tmp;
+}
+


Test a few more functions for good measure? Also test some cases with out arguments, like frexp.

Also test sincos? it has out float arguments so I'm not sure you apply this

I add the extra frexp and sincos, but find that both of them don't generate tbaa info with this PR.
a) frexp is similar to fabs, it is not subject to any errors
b) sincos is not a builtin function

frexp isn't subject to any errors, but also writes to its int pointer out argument, so it could

Oh, thanks, I'll adjust Negative test to TODO

arsenm · 2024-07-28T08:01:16Z

clang/test/CodeGen/math-libcalls-tbaa.cpp

 //
-extern "C" float foo (float num[], float r2inv, int n) {
-   const float expm2 =  expf(num[10]);  // Emit TBAA metadata on @expf
+extern "C" float foo (float num[]) {


Could also just make this a C test instead of all the extern Cs?

Done, thanks

arsenm · 2024-07-29T14:02:37Z

clang/lib/CodeGen/CGBuiltin.cpp

+    if (ConstWithoutErrnoAndExceptions && CGF.CGM.getLangOpts().MathErrno &&
        !CGF.Builder.getIsFPConstrained()) {


Make this the last check in this expression

arsenm · 2024-07-29T14:04:07Z

clang/test/CodeGen/math-libcalls-tbaa.c

+// CHECK-NEXT:    call void @llvm.lifetime.start.p0(i64 4, ptr nonnull [[COS]]) #[[ATTR9]]
+// CHECK-NEXT:    [[ARRAYIDX:%.*]] = getelementptr inbounds i8, ptr [[NUM]], i64 8
+// CHECK-NEXT:    [[TMP0:%.*]] = load float, ptr [[ARRAYIDX]], align 4, !tbaa [[TBAA2]]
+// CHECK-NEXT:    call void @sincos(float noundef [[TMP0]], ptr noundef nonnull [[SIN]], ptr noundef nonnull [[COS]]) #[[ATTR9]]


sincos can set errno, so I'm not sure how this managed to skip the tbaa check

It is the checking callee.isBuiltin() , so need another patch to add sincos as a buildin ?

llvm-project/clang/lib/CodeGen/CGExpr.cpp

Line 5467 in 3a9ef4e

if (callee.isBuiltin()) {

vfdff · 2024-07-30T04:04:14Z

If a builtin return a complex type, then it can not be convert into a Instruction* type, so add a new restriction Call.isScalar()

arsenm · 2024-07-30T04:34:45Z

clang/test/CodeGen/math-libcalls-tbaa.c

+// CHECK-NEXT:    ret float [[MUL]]
+//
+float foo (float num[]) {
+   const float expm2 = expf(num[10]);  // Emit TBAA metadata on @expf


Should also test with __builtin

Done, add foo_buildin

arsenm · 2024-07-30T04:35:56Z

clang/lib/CodeGen/CGBuiltin.cpp

-      ASTContext &Context = CGF.getContext();
+    // TODO: Support builtin function with complex type returned, eg: cacosh
+    if (ConstWithoutErrnoAndExceptions && CGF.CGM.getLangOpts().MathErrno &&
+        !CGF.Builder.getIsFPConstrained() && Call.isScalar()) {


Need test with the complex type

Thanks, add foo_cacoshf

arsenm

lgtm with test cleanup

arsenm · 2024-07-30T08:23:50Z

clang/test/CodeGen/math-libcalls-tbaa.c

+// CHECK-NEXT:    [[MUL:%.*]] = fmul float [[TMP0]], [[CALL]]
+// CHECK-NEXT:    ret float [[MUL]]
+//
+float foo_buildin (float num[]) {


Fix testname. Typo building, and name the function tested instead of just having a foo suffix

Apply your comment, thanks.

arsenm · 2024-07-30T10:56:29Z

clang/test/CodeGen/math-libcalls-tbaa.c

+// CHECK-NEXT:    [[MUL:%.*]] = fmul double [[TMP0]], [[CALL]]
+// CHECK-NEXT:    ret double [[MUL]]
+//
+double foo_remainder (double num[], double a) {


Remove the foos

the function name remove the foo suffix will conflict to the related math library name, so do you mean adjust the test function name foo_expf to _expf for example?

How about "test"

arsenm · 2024-07-30T10:56:50Z

clang/test/CodeGen/math-libcalls-tbaa.c

+// RUN:  %clang_cc1 -triple=aarch64-unknown-linux-gnu -fmath-errno -O3 -emit-llvm -o - %s | FileCheck %s -check-prefixes=CHECK,NoNewStructPathTBAA
+// RUN:  %clang_cc1 -triple=aarch64-unknown-linux-gnu -fmath-errno -O3 -new-struct-path-tbaa -emit-llvm -o - %s | FileCheck %s -check-prefixes=CHECK,NewStructPathTBAA


Usually check prefixes are all caps

[clang codegen] Emit int TBAA metadata on more FP math libcalls

c750234

Follow PR96025, except expf, more FP math libcalls in libm should also be supported. Fix llvm#86635

vfdff requested review from arsenm, efriedma-quic, jdoerfert and zahiraam July 24, 2024 05:13

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Jul 24, 2024

arsenm requested changes Jul 24, 2024

View reviewed changes

Add a new test calls fabs

fb81f30

arsenm reviewed Jul 24, 2024

View reviewed changes

Update with isConstWithoutErrnoAndExceptions

b4cbc05

vfdff requested a review from arsenm July 27, 2024 10:44

arsenm reviewed Jul 28, 2024

View reviewed changes

Address comments

ce42e64

arsenm reviewed Jul 29, 2024

View reviewed changes

workaroud the unsupported complex type builtin

8d80433

arsenm reviewed Jul 30, 2024

View reviewed changes

Add 2 tests builtin and _Complex type

8ea8766

arsenm reviewed Jul 30, 2024

View reviewed changes

Fix testname

e03ab41

arsenm reviewed Jul 30, 2024

View reviewed changes

Rename test function

4770e71

arsenm approved these changes Jul 30, 2024

View reviewed changes

vfdff merged commit 9589c12 into llvm:main Jul 31, 2024

wsmoses mentioned this pull request Aug 5, 2024

[TBAA] Invalid TBAA added to libcalls #102029

Closed

zahiraam mentioned this pull request Sep 6, 2024

Don't emit int TBAA metadata on more complex FP math libcalls. #107598

Closed

MacDue mentioned this pull request Sep 17, 2024

[clang][codegen] Don't mark "int" TBAA on FP libcalls with indirect args #108853

Merged

		if (ConstWithoutErrnoAndExceptions && CGF.CGM.getLangOpts().MathErrno &&
		!CGF.Builder.getIsFPConstrained()) {

		// RUN: %clang_cc1 -triple=aarch64-unknown-linux-gnu -fmath-errno -O3 -emit-llvm -o - %s \| FileCheck %s -check-prefixes=CHECK,NoNewStructPathTBAA
		// RUN: %clang_cc1 -triple=aarch64-unknown-linux-gnu -fmath-errno -O3 -new-struct-path-tbaa -emit-llvm -o - %s \| FileCheck %s -check-prefixes=CHECK,NewStructPathTBAA

[clang codegen] Emit int TBAA metadata on more FP math libcalls #100302

[clang codegen] Emit int TBAA metadata on more FP math libcalls #100302

Uh oh!

Conversation

vfdff commented Jul 24, 2024 • edited by arsenm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

vfdff commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdff Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdff commented Jul 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdff Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdff commented Jul 24, 2024 •

edited by arsenm

Loading

llvmbot commented Jul 24, 2024 •

edited

Loading

vfdff Jul 25, 2024 •

edited

Loading

vfdff Jul 30, 2024 •

edited

Loading