[clang][CodeGen] Propagate pragma set fast-math flags to floating point builtins #90377

karka228 · 2024-04-28T06:43:15Z

This is a fix for the issue #87758 where fast-math flags are not propagated all builtins.

It seems like pragmas with fast math flags was only propagated to calls of unary floating point builtins. This patch propagate them also for binary and ternary floating point builtins.

Adding test case related to issue llvm#87758 The testcase show the faulty behavior where the calls to llvm.pow.f32 and llvm.fma.f32 are not attributed with the "fast" flag.

…nt builtins This is a fix for the issue llvm#87758 where fast-math flags are not propagated all builtins. It seems like pragmas with fast math flags was only propagated to calls of unary floating point builtins. This patch propagate them also for binary and ternary floating point builtins.

llvmbot · 2024-04-28T06:43:45Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Karl-Johan Karlsson (karka228)

Changes

This is a fix for the issue #87758 where fast-math flags are not propagated all builtins.

It seems like pragmas with fast math flags was only propagated to calls of unary floating point builtins. This patch propagate them also for binary and ternary floating point builtins.

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGBuiltin.cpp (+3-3)
(added) clang/test/CodeGen/pr87758.c (+28)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index d08ab539148914..d61a63ac61572d 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -513,8 +513,8 @@ static Value *emitBinaryMaybeConstrainedFPBuiltin(CodeGenFunction &CGF,
   llvm::Value *Src0 = CGF.EmitScalarExpr(E->getArg(0));
   llvm::Value *Src1 = CGF.EmitScalarExpr(E->getArg(1));
 
+  CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
   if (CGF.Builder.getIsFPConstrained()) {
-    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
     Function *F = CGF.CGM.getIntrinsic(ConstrainedIntrinsicID, Src0->getType());
     return CGF.Builder.CreateConstrainedFPCall(F, { Src0, Src1 });
   } else {
@@ -530,8 +530,8 @@ static Value *emitBinaryExpMaybeConstrainedFPBuiltin(
   llvm::Value *Src0 = CGF.EmitScalarExpr(E->getArg(0));
   llvm::Value *Src1 = CGF.EmitScalarExpr(E->getArg(1));
 
+  CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
   if (CGF.Builder.getIsFPConstrained()) {
-    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
     Function *F = CGF.CGM.getIntrinsic(ConstrainedIntrinsicID,
                                        {Src0->getType(), Src1->getType()});
     return CGF.Builder.CreateConstrainedFPCall(F, {Src0, Src1});
@@ -551,8 +551,8 @@ static Value *emitTernaryMaybeConstrainedFPBuiltin(CodeGenFunction &CGF,
   llvm::Value *Src1 = CGF.EmitScalarExpr(E->getArg(1));
   llvm::Value *Src2 = CGF.EmitScalarExpr(E->getArg(2));
 
+  CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
   if (CGF.Builder.getIsFPConstrained()) {
-    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
     Function *F = CGF.CGM.getIntrinsic(ConstrainedIntrinsicID, Src0->getType());
     return CGF.Builder.CreateConstrainedFPCall(F, { Src0, Src1, Src2 });
   } else {
diff --git a/clang/test/CodeGen/pr87758.c b/clang/test/CodeGen/pr87758.c
new file mode 100644
index 00000000000000..05b3232986e0a6
--- /dev/null
+++ b/clang/test/CodeGen/pr87758.c
@@ -0,0 +1,28 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -emit-llvm -o - %s \
+// RUN: | FileCheck -check-prefix=CHECK %s
+
+// Reproducer for issue #87758
+// The testcase below verifies that the "fast" flag are set on the calls.
+
+float sqrtf(float x);
+float powf(float x, float y);
+float fmaf(float x, float y, float z);
+
+#pragma float_control(push)
+#pragma float_control(precise, off)
+// CHECK-LABEL: define dso_local float @fp_precise_libm_calls(
+// CHECK-SAME: float noundef [[A:%.*]], float noundef [[B:%.*]], float noundef [[C:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[TMP0:%.*]] = tail call fast float @llvm.sqrt.f32(float [[A]])
+// CHECK-NEXT:    [[TMP1:%.*]] = tail call fast float @llvm.pow.f32(float [[TMP0]], float [[B]])
+// CHECK-NEXT:    [[TMP2:%.*]] = tail call fast float @llvm.fma.f32(float [[TMP1]], float [[B]], float [[C]])
+// CHECK-NEXT:    ret float [[TMP2]]
+//
+float fp_precise_libm_calls(float a, float b, float c) {
+  a = sqrtf(a);
+  a = powf(a,b);
+  a = fmaf(a,b,c);
+  return a;
+}
+#pragma float_control(pop)

andykaylor

This looks right to me, but @zahiraam is more familiar with this code than I am. Zahira, do you have any comments?

zahiraam · 2024-04-29T18:13:33Z

clang/test/CodeGen/pr87758.c

@@ -0,0 +1,28 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -emit-llvm -o - %s \


There is no need to use -O3 here to prove what you want, I think.

The only reason for using -O3 was to get rid of all the allocas, loads and stores in the output that didn't contributed to the test.

You don't need to put every // CHECK line. Since we are interested only in the flag associated with the calls to the builtins you can just put those line.

zahiraam · 2024-04-29T18:31:45Z

clang/test/CodeGen/pr87758.c

+  a = fmaf(a,b,c);
+  return a;
+}
+#pragma float_control(pop)


You could also use the same test with #pragma float_control(precise,on) and confirm that the fast flags are not set.

By combining #pragma float_control(precise,on) and -ffast-math to the testcase it revealed additional bugs, as the "fast" flag was still set. I tried to fix this by changing the function emitLibraryCall() but I'm a bit unsure if this is a correct fix.

I tried the patch with your change in emitLibraryCall() and it seems to be fixing at least one known issue.
For the code here (using your 3 functions from this PR): https://godbolt.org/z/fzsGce76x

We can see that with -ffp-model=fast all function calls in foof function are correctly marked with the fast flag, but in foop they are incorrectly marked with the fast flag.
When compiled with ffp-model=precise some function calls are missing the fast flag in foof.
With your patch all these issues are corrected. Please confirm that. And if it's the case, then I think you can keep your last change and add more testing.

I updated the testcase with more RUN: lines and also added a libc call to rindex that have nothing to do with floats to verify that it never get the "fast" flag.

zahiraam · 2024-04-29T18:32:57Z

clang/test/CodeGen/pr87758.c

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -emit-llvm -o - %s \
+// RUN: | FileCheck -check-prefix=CHECK %s
+


How about adding a run line with ffast-math option?

Sure, I will add that.

…nt builtins Minor fix in emitLibraryCall() and updated testcase.

…nt builtins Updated testcase.

zahiraam · 2024-05-03T12:57:16Z

clang/test/CodeGen/pr87758.c

@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -fmath-errno -ffp-contract=on -fno-rounding-math -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s


This is equivalent to line 7.

zahiraam · 2024-05-03T12:57:59Z

clang/test/CodeGen/pr87758.c

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -fmath-errno -ffp-contract=on -fno-rounding-math -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffp-contract=fast -emit-llvm -o - %s | FileCheck %s


This is equivalent to line 8.

zahiraam · 2024-05-03T13:06:58Z

clang/test/CodeGen/pr87758.c

+
+// Reproducer for issue #87758
+// The testcase below verifies that the "fast" flag are set on the calls.
+


How about using these run lines?
// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck %s

// precise mode
// RUN: %clang_cc1 -triple x86_64-linux-gnu -fmath-errno -ffp-contract=on \
// RUN: -fno-rounding-math -emit-llvm -o - %s | FileCheck %s

// fast mode
// RUN %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffp-contract=fast \
// RUN: -emit-llvm -o - %s | FileCheck %s
You might need to use different prefixes.

Sure, I update the testcase with only those run lines.

…nt builtins Updated testcase.

zahiraam · 2024-05-03T19:22:56Z

clang/test/CodeGen/pr87758.c

+#pragma float_control(push)
+#pragma float_control(precise, off)
+// CHECK: define dso_local float @fp_precise_off_libm_calls(
+// CHECK: %{{.*}} = call fast float @llvm.sqrt.f32(


Do we need the %{{.*}} = before these calls?

I guess not any longer. I'll change the testcase.

…nt builtins Updated testcase.

zahiraam

LGTM. Thanks.

karka228 · 2024-05-05T12:31:05Z

LGTM. Thanks.

I will close the issue #87758.

Thanks @andykaylor and @zahiraam for reviewing.

karka228 added 2 commits April 27, 2024 22:10

Pre-commit reproducer for fast-math flags on calls

80836f5

Adding test case related to issue llvm#87758 The testcase show the faulty behavior where the calls to llvm.pow.f32 and llvm.fma.f32 are not attributed with the "fast" flag.

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

karka228 requested a review from andykaylor April 28, 2024 08:45

andykaylor requested a review from zahiraam April 29, 2024 17:15

andykaylor approved these changes Apr 29, 2024

View reviewed changes

zahiraam reviewed Apr 29, 2024

View reviewed changes

karka228 added 2 commits April 30, 2024 15:02

[clang][CodeGen] Propagate pragma set fast-math flags to floating poi…

e1b01ae

…nt builtins Minor fix in emitLibraryCall() and updated testcase.

[clang][CodeGen] Propagate pragma set fast-math flags to floating poi…

251009f

…nt builtins Updated testcase.

zahiraam reviewed May 3, 2024

View reviewed changes

[clang][CodeGen] Propagate pragma set fast-math flags to floating poi…

dc28827

…nt builtins Updated testcase.

zahiraam reviewed May 3, 2024

View reviewed changes

[clang][CodeGen] Propagate pragma set fast-math flags to floating poi…

af625c7

…nt builtins Updated testcase.

zahiraam approved these changes May 3, 2024

View reviewed changes

karka228 merged commit cb015b9 into llvm:main May 4, 2024

		@@ -0,0 +1,28 @@
		// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
		// RUN: %clang_cc1 -triple x86_64-linux-gnu -O3 -emit-llvm -o - %s \


		// Reproducer for issue #87758
		// The testcase below verifies that the "fast" flag are set on the calls.

[clang][CodeGen] Propagate pragma set fast-math flags to floating point builtins #90377

[clang][CodeGen] Propagate pragma set fast-math flags to floating point builtins #90377

Uh oh!

Conversation

karka228 commented Apr 28, 2024

Uh oh!

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

Uh oh!

Uh oh!

andykaylor 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!

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!

Choose a reason for hiding this comment

Uh oh!

zahiraam left a comment

Choose a reason for hiding this comment

Uh oh!

karka228 commented May 5, 2024

Uh oh!

Uh oh!

llvmbot commented Apr 28, 2024 •

edited

Loading