[clang] Set FPOptions at the beginning of CompoundStmt

[spavloff]

CompoundStmt has FPOptions, that should be set for IRBuilder when generating code if that statement.

It must fix the issue #84648.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Serge Pavlov (spavloff)

Changes

CompoundStmt has FPOptions, that should be set for IRBuilder when generating code if that statement.

It must fix the issue #84648.

---

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

3 Files Affected:

• (modified) clang/include/clang/AST/Stmt.h (+9)
• (modified) clang/lib/CodeGen/CGStmt.cpp (+5)
• (modified) clang/test/CodeGen/fast-math.c (+17-1)

diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 83fafbabb1d460..805148bce4aff1 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1684,6 +1684,15 @@ class CompoundStmt final
     return hasStoredFPFeatures() ? getStoredFPFeatures() : FPOptionsOverride();
   }
 
+  /// Get FPOptions inside this statement. They may differ from the outer
+  /// options due to pragmas.
+  /// \param CurFPOptions FPOptions outside this statement.
+  FPOptions getInsideFPOptions(FPOptions CurFPOptions) const {
+    return hasStoredFPFeatures()
+               ? getStoredFPFeatures().applyOverrides(CurFPOptions)
+               : CurFPOptions;
+  }
+
   using body_iterator = Stmt **;
   using body_range = llvm::iterator_range<body_iterator>;
 
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 41dc91c578c800..da9bf76a54f963 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -522,6 +522,11 @@ CodeGenFunction::EmitCompoundStmtWithoutScope(const CompoundStmt &S,
   assert((!GetLast || (GetLast && ExprResult)) &&
          "If GetLast is true then the CompoundStmt must have a StmtExprResult");
 
+  // Optionally set up the new FP environment, if the compound statement
+  // contains a pragma that modifies it.
+  FPOptions NewFP = S.getInsideFPOptions(CurFPFeatures);
+  CGFPOptionsRAII SavedFPFeatues(*this, NewFP);
+
   Address RetAlloca = Address::invalid();
 
   for (auto *CurStmt : S.body()) {
diff --git a/clang/test/CodeGen/fast-math.c b/clang/test/CodeGen/fast-math.c
index 6ebd65a22c92be..048a5aeb9649ba 100644
--- a/clang/test/CodeGen/fast-math.c
+++ b/clang/test/CodeGen/fast-math.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -ffast-math -ffp-contract=fast -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -ffast-math -ffp-contract=fast -emit-llvm -O2 -o - %s | FileCheck %s
 float f0, f1, f2;
 
 void foo(void) {
@@ -9,3 +9,19 @@ void foo(void) {
 
   // CHECK: ret
 }
+
+float issue_84648a(float *x) {
+  return x[0] == x[1] ? x[1] : x[0];
+}
+
+// CHECK-LABEL: define{{.*}} float @issue_84648a(ptr {{.*}})
+// CHECK:       [[VAL:%.+]] = load float, ptr
+// CHECK:       ret float [[VAL]]
+
+float issue_84648b(float *x) {
+#pragma float_control(precise, on)
+  return x[0] == x[1] ? x[1] : x[0];
+}
+
+// CHECK-LABEL: define{{.*}} float @issue_84648b(ptr{{.*}} %x)
+// CHECK:       fcmp oeq

[spavloff]

Ping.

[zahiraam]

You seem to have some fails in the CI.

[zahiraam]

How about giving another name to this function? Something like getStoredFPFeaturesInStmt or getStoredFPFeaturesInCompoundStmt.

[spavloff]

I renamed it to getActiveFPOptions. If you have ideas how to make it better, please share.

A name that refers to StoredFPFeatures is a bit misleading, because AST nodes already have method getStoredFPFeatures, which reports the stored FP features in the form of FPOptionsOverride.

[zahiraam]

It seems to me that this issue exists only at O2, right? https://godbolt.org/z/xGcWG1aM6
Shouldn't that be reflected in the code somewhere?

[spavloff]

Yes, this fact is reflected in #84648. Actually the problem is incorrect optimization caused by wrong fast-math flags.

Also your example cannot demonstrate the issue because it is C++ code. In C++ mode instructions like SelectInst or PHINode do not get fast-math flags.

[rjmccall]

I'm sorry if you've explained this before, but I thought our general approach was to just store the options in individual expressions, and CodeGen should in general already be paying attention to those options. Is there a specific reason we also need to honor these at the compound statement level? If so, is there an expected invariant we can take advantage of to avoid all the bookkeeping work we end up doing when creating and emitting individual expressions?

[spavloff]

FPOptions were added to CompoundStmt to help code generation for #pragma STDC FENV_ROUND. If dynamic rounding is used, compiler should emit a code that switch it. There is no AST nodes for pragmas, so there is no a good place to emit such code. To solve this problem CompoundStmt was allowed to have FPOption, although it is not an expression.

This patch is a part of #89617, which only sets FPOptions for statements inside a compound statement that do not have FPOptions attached.

Now fast-math flags may be attached to SelectInst and PHINode, so at some point someone could want to attach FPOptions to, say, IfStmt or any other control statement. This solution hopefully could help preventing from adding FPOptions to the nodes that are not floating-point operations.