[FPEnv] Add strictfp in some C++ constructors lacking a FunctionDecl. #74883

kpneal · 2023-12-08T20:05:52Z

Some C++ constructors require the strictfp attribute on all function calls but don't get it because the comstructor lacks a FunctionDecl. Use the IRBuilder's strictfp mode to communicate that the strictfp attribute is required. Note that the function calls that were missing the attribute were getting it from the IRBuilder but it then was getting lost when CGCall.cpp replaced all the attributes for the function call.

Verified with "https://reviews.llvm.org/D146845".

Some C++ constructors require the strictfp attribute on all function calls but don't get it because the comstructor lacks a FunctionDecl. Use the IRBuilder's strictfp mode to communicate that the strictfp attribute is required. Note that the function calls that were missing the attribute were getting it from the IRBuilder but it then was getting lost when CGCall.cpp replaced all the attributes for the function call. Verified with "https://reviews.llvm.org/D146845".

llvmbot · 2023-12-08T20:06:22Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Kevin P. Neal (kpneal)

Changes

Some C++ constructors require the strictfp attribute on all function calls but don't get it because the comstructor lacks a FunctionDecl. Use the IRBuilder's strictfp mode to communicate that the strictfp attribute is required. Note that the function calls that were missing the attribute were getting it from the IRBuilder but it then was getting lost when CGCall.cpp replaced all the attributes for the function call.

Verified with "https://reviews.llvm.org/D146845".

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

4 Files Affected:

(modified) clang/include/clang/AST/ExprCXX.h (+4)
(modified) clang/lib/CodeGen/CGCall.cpp (+8)
(modified) clang/lib/CodeGen/CGDeclCXX.cpp (+5)
(modified) clang/test/CodeGen/fp-floatcontrol-class.cpp (+71-4)

diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 24278016431837..f9269082c32475 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -1698,6 +1698,10 @@ class CXXConstructExpr : public Expr {
   SourceRange getParenOrBraceRange() const { return ParenOrBraceRange; }
   void setParenOrBraceRange(SourceRange Range) { ParenOrBraceRange = Range; }
 
+  /// Determine whether the function was declared in source context
+  /// that requires constrained FP intrinsics
+  bool UsesFPIntrin() const { return Constructor->UsesFPIntrin(); }
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CXXConstructExprClass ||
            T->getStmtClass() == CXXTemporaryObjectExprClass;
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index a24aeea7ae32bf..7d1bb4dc75ab45 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -5520,6 +5520,12 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
       CGM.AdjustMemoryAttribute(CalleePtr->getName(), Callee.getAbstractInfo(),
                                 Attrs);
   }
+  // We may not have a FunctionDecl*, but we still need to support strictfp.
+  if (Builder.getIsFPConstrained()) {
+    // All calls within a strictfp function are marked strictfp
+    Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::StrictFP);
+  }
+
   // Add call-site nomerge attribute if exists.
   if (InNoMergeAttributedStmt)
     Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoMerge);
@@ -5587,10 +5593,12 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
       !isa_and_nonnull<FunctionDecl>(TargetDecl))
     EmitKCFIOperandBundle(ConcreteCallee, BundleList);
 
+#if 0 // XXX Why is this here? Duplicate!
   if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl))
     if (FD->hasAttr<StrictFPAttr>())
       // All calls within a strictfp function are marked strictfp
       Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::StrictFP);
+#endif
 
   AssumeAlignedAttrEmitter AssumeAlignedAttrEmitter(*this, TargetDecl);
   Attrs = AssumeAlignedAttrEmitter.TryEmitAsCallSiteAttribute(Attrs);
diff --git a/clang/lib/CodeGen/CGDeclCXX.cpp b/clang/lib/CodeGen/CGDeclCXX.cpp
index e08a1e5f42df20..5700b7fcb49d32 100644
--- a/clang/lib/CodeGen/CGDeclCXX.cpp
+++ b/clang/lib/CodeGen/CGDeclCXX.cpp
@@ -1012,6 +1012,11 @@ void CodeGenFunction::GenerateCXXGlobalVarDeclInitFunc(llvm::Function *Fn,
 
   CurEHLocation = D->getBeginLoc();
 
+  if (const auto *CE = dyn_cast<CXXConstructExpr>(D->getInit())) {
+    if (CE->UsesFPIntrin())
+      Builder.setIsFPConstrained(true);
+  }
+
   StartFunction(GlobalDecl(D, DynamicInitKind::Initializer),
                 getContext().VoidTy, Fn, getTypes().arrangeNullaryFunction(),
                 FunctionArgList());
diff --git a/clang/test/CodeGen/fp-floatcontrol-class.cpp b/clang/test/CodeGen/fp-floatcontrol-class.cpp
index 83a27cb206eb82..c9953119fd3a22 100644
--- a/clang/test/CodeGen/fp-floatcontrol-class.cpp
+++ b/clang/test/CodeGen/fp-floatcontrol-class.cpp
@@ -1,3 +1,4 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --include-generated-funcs --version 4
 // RUN: %clang_cc1 -ffp-contract=on -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck %s
 // Verify that float_control does not pertain to initializer expressions
 
@@ -6,14 +7,80 @@ float z();
 #pragma float_control(except, on)
 class ON {
   float w = 2 + y() * z();
-  // CHECK-LABEL: define {{.*}} @_ZN2ONC2Ev{{.*}}
-  // CHECK: llvm.experimental.constrained.fmul{{.*}}tonearest{{.*}}strict
 };
 ON on;
 #pragma float_control(except, off)
 class OFF {
   float w = 2 + y() * z();
-  // CHECK-LABEL: define {{.*}} @_ZN3OFFC2Ev{{.*}}
-  // CHECK-NOT: llvm.experimental.constrained.fmul{{.*}}tonearest{{.*}}strict
 };
 OFF off;
+// CHECK-LABEL: define internal void @__cxx_global_var_init(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] section ".text.startup" {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    call void @_ZN2ONC1Ev(ptr noundef nonnull align 4 dereferenceable(4) @on) #[[ATTR6:[0-9]+]]
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN2ONC1Ev(
+// CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(4) [[THIS:%.*]]) unnamed_addr #[[ATTR1:[0-9]+]] comdat align 2 {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    [[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    call void @_ZN2ONC2Ev(ptr noundef nonnull align 4 dereferenceable(4) [[THIS1]]) #[[ATTR6]]
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK-LABEL: define internal void @__cxx_global_var_init.1(
+// CHECK-SAME: ) #[[ATTR0]] section ".text.startup" {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    call void @_ZN3OFFC1Ev(ptr noundef nonnull align 4 dereferenceable(4) @off)
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN3OFFC1Ev(
+// CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(4) [[THIS:%.*]]) unnamed_addr #[[ATTR2:[0-9]+]] comdat align 2 {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    [[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    call void @_ZN3OFFC2Ev(ptr noundef nonnull align 4 dereferenceable(4) [[THIS1]])
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN2ONC2Ev(
+// CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(4) [[THIS:%.*]]) unnamed_addr #[[ATTR1]] comdat align 2 {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    [[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    [[W:%.*]] = getelementptr inbounds [[CLASS_ON:%.*]], ptr [[THIS1]], i32 0, i32 0
+// CHECK-NEXT:    [[CONV:%.*]] = call float @llvm.experimental.constrained.sitofp.f32.i32(i32 2, metadata !"round.tonearest", metadata !"fpexcept.strict") #[[ATTR6]]
+// CHECK-NEXT:    [[CALL:%.*]] = call noundef float @_Z1yv() #[[ATTR6]]
+// CHECK-NEXT:    [[CALL2:%.*]] = call noundef float @_Z1zv() #[[ATTR6]]
+// CHECK-NEXT:    [[TMP0:%.*]] = call float @llvm.experimental.constrained.fmuladd.f32(float [[CALL]], float [[CALL2]], float [[CONV]], metadata !"round.tonearest", metadata !"fpexcept.strict") #[[ATTR6]]
+// CHECK-NEXT:    store float [[TMP0]], ptr [[W]], align 4
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN3OFFC2Ev(
+// CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(4) [[THIS:%.*]]) unnamed_addr #[[ATTR2]] comdat align 2 {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    [[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// CHECK-NEXT:    [[W:%.*]] = getelementptr inbounds [[CLASS_OFF:%.*]], ptr [[THIS1]], i32 0, i32 0
+// CHECK-NEXT:    [[CALL:%.*]] = call noundef float @_Z1yv()
+// CHECK-NEXT:    [[CALL2:%.*]] = call noundef float @_Z1zv()
+// CHECK-NEXT:    [[TMP0:%.*]] = call float @llvm.fmuladd.f32(float [[CALL]], float [[CALL2]], float 2.000000e+00)
+// CHECK-NEXT:    store float [[TMP0]], ptr [[W]], align 4
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK-LABEL: define internal void @_GLOBAL__sub_I_fp_floatcontrol_class.cpp(
+// CHECK-SAME: ) #[[ATTR0]] section ".text.startup" {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    call void @__cxx_global_var_init()
+// CHECK-NEXT:    call void @__cxx_global_var_init.1()
+// CHECK-NEXT:    ret void
+//

rjmccall · 2023-12-08T20:31:51Z

clang/lib/CodeGen/CGCall.cpp

  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl))
    if (FD->hasAttr<StrictFPAttr>())
      // All calls within a strictfp function are marked strictfp
      Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::StrictFP);
+#endif


Please remove this instead of #if 0ing it.

rjmccall · 2023-12-08T20:33:23Z

clang/test/CodeGen/fp-floatcontrol-class.cpp

 };
 OFF off;
+// CHECK-LABEL: define internal void @__cxx_global_var_init(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] section ".text.startup" {


Are all of these checks necessary? If so, please explain why in the test so that maintainers will understand what you're testing for.

Do you need to be checking the actual content of the attributes here?

I missed the argument to update_cc_test_checks.py that includes the attribute comments. Fixed. Is the new comment about checking for strictfp sufficient? Or do I need to include a mention of the documentation in the LangRef?

So here's the problem with these autogenerated checks: you're testing for a bunch of stuff you don't really care about and which we'll probably regret testing for in the future, and you've also lost some of the expressive intent of the old test. Like, you don't actually care that the this argument is noundef nonnull align 4 dereferenceable(4), and when we inevitably come up with yet another attribute to slap on this, we'll have to modify this test for no good reason. Meanwhile, the old test was pretty clear that it was testing whether the arithmetic in the constructor is done with constrained FP intrinsics, because the check lines were right next to the code involved, and now that isn't obvious at all.

In general:

Please position checks in the test file so that they're clearly associated with the code that they're testing.

Please edit the autogenerated checks down to make sure they're just checking the things you actually care about.

Please think about how other programmers will read this test and understand what you're testing for.

In specific, the only new thing you're really testing for here is, what, that the strictfp attribute is on _ZN2ONC2Ev and not on _ZN3OFFC2Ev? Can you test for that?

unnecessary code.

rjmccall · 2023-12-14T20:03:11Z

clang/lib/CodeGen/CGCall.cpp

@@ -5520,6 +5520,12 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
      CGM.AdjustMemoryAttribute(CalleePtr->getName(), Callee.getAbstractInfo(),
                                Attrs);
  }
+  // We may not have a FunctionDecl*, but we still need to support strictfp.


This comment is explaining the patch, not explaining the code as it stands after the patch. I suggest removing this and just pulling the comment inside the if up here, since it explains why this block has to exist.

rjmccall · 2023-12-14T20:25:05Z

clang/test/CodeGen/fp-floatcontrol-class.cpp

 };
 OFF off;
+// CHECK-LABEL: define internal void @__cxx_global_var_init(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] section ".text.startup" {


So here's the problem with these autogenerated checks: you're testing for a bunch of stuff you don't really care about and which we'll probably regret testing for in the future, and you've also lost some of the expressive intent of the old test. Like, you don't actually care that the this argument is noundef nonnull align 4 dereferenceable(4), and when we inevitably come up with yet another attribute to slap on this, we'll have to modify this test for no good reason. Meanwhile, the old test was pretty clear that it was testing whether the arithmetic in the constructor is done with constrained FP intrinsics, because the check lines were right next to the code involved, and now that isn't obvious at all.

In general:

Please position checks in the test file so that they're clearly associated with the code that they're testing.

Please edit the autogenerated checks down to make sure they're just checking the things you actually care about.

Please think about how other programmers will read this test and understand what you're testing for.

In specific, the only new thing you're really testing for here is, what, that the strictfp attribute is on _ZN2ONC2Ev and not on _ZN3OFFC2Ev? Can you test for that?

rjmccall

I'm sorry for failing to notice this in my previous review, but I don't think this patch is right. A global initializer should be FP-constrained based on whether the variable is defined in an FP-constrained context. This is testing whether the outermost level of the initializer is a call to an FP-constrained constructor. That means that we will not treat the initializer as FP constrained if it's not of class type or if it calls a non-FP-constrained constructor but passes arguments that require FP-constrained code emission. (It will also be wrong if the expression requires temporaries, because then the expression will not be directly a CXXConstructExpr.)

If we don't currently capture in the AST whether a global variable definition is in an FP-constrained context, that seems like something we should fix.

kpneal · 2024-03-08T16:13:07Z

OK, I'll head in the direction you've pointed. Thanks for your time!

kpneal requested review from rjmccall, spavloff and mcinally December 8, 2023 20:05

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen labels Dec 8, 2023

rjmccall reviewed Dec 8, 2023

View reviewed changes

kpneal added 2 commits December 13, 2023 15:16

Update for review comments. Add missing bits to test case, eliminate

40b4c51

unnecessary code.

Add a comment on the strictfp use of this test.

823baf7

rjmccall reviewed Dec 14, 2023

View reviewed changes

kpneal added 2 commits January 10, 2024 13:29

Move comments as requested in review.

37f6d7f

Update test to be more precise in what it tests.

8b1ef1a

rjmccall requested changes Jan 22, 2024

View reviewed changes

kpneal closed this Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FPEnv] Add strictfp in some C++ constructors lacking a FunctionDecl. #74883

[FPEnv] Add strictfp in some C++ constructors lacking a FunctionDecl. #74883

kpneal commented Dec 8, 2023

llvmbot commented Dec 8, 2023 •

edited

rjmccall Dec 8, 2023

kpneal Dec 14, 2023

rjmccall Dec 8, 2023

kpneal Dec 14, 2023

rjmccall Dec 14, 2023

rjmccall Dec 14, 2023

rjmccall Dec 14, 2023

rjmccall left a comment

kpneal commented Mar 8, 2024

[FPEnv] Add strictfp in some C++ constructors lacking a FunctionDecl. #74883

[FPEnv] Add strictfp in some C++ constructors lacking a FunctionDecl. #74883

Conversation

kpneal commented Dec 8, 2023

llvmbot commented Dec 8, 2023 • edited

rjmccall Dec 8, 2023

Choose a reason for hiding this comment

kpneal Dec 14, 2023

Choose a reason for hiding this comment

rjmccall Dec 8, 2023

Choose a reason for hiding this comment

kpneal Dec 14, 2023

Choose a reason for hiding this comment

rjmccall Dec 14, 2023

Choose a reason for hiding this comment

rjmccall Dec 14, 2023

Choose a reason for hiding this comment

rjmccall Dec 14, 2023

Choose a reason for hiding this comment

rjmccall left a comment

Choose a reason for hiding this comment

kpneal commented Mar 8, 2024

llvmbot commented Dec 8, 2023 •

edited