clang: Stop emitting "strictfp"

[arsenm]

Summary

The attribute is a proper enum attribute, strictfp. We were getting
strictfp and "strictfp" set on every function with
-fexperimental-strict-floating-point.

[kpn]

It looks like clang does have at least one test that checks for the strictfp attribute on function definitions. We also have a number that test for the strictfp attribute on function calls. So I think our test coverage is in good shape.

LGTM.

[zahiraam]

Can you please be more verbose in your summary? This change is related to the use of -ffp-exception-behavior=strict right?
This attribute is set here https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.cpp#L987 but under a condition. Is that condition always satisfied when the attribute needs to be set?

[arsenm]

In D139629#3981991, @zahiraam wrote:

Can you please be more verbose in your summary? This change is related to the use of -ffp-exception-behavior=strict right?
This attribute is set here https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.cpp#L987 but under a condition. Is that condition always satisfied when the attribute needs to be set?

I don't know if there are other cases that misses; if so it's broken anyway. Nothing reads this string formed version

[arsenm]

In D139629#3982052, @arsenm wrote:

In D139629#3981991, @zahiraam wrote:

Can you please be more verbose in your summary? This change is related to the use of -ffp-exception-behavior=strict right?
This attribute is set here https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.cpp#L987 but under a condition. Is that condition always satisfied when the attribute needs to be set?

I don't know if there are other cases that misses; if so it's broken anyway. Nothing reads this string formed version

It's coming from the FunctionDecl and the other case is broader. Only question would be of reachability

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

2135

I think it would better to fix this function instead of removing it entirely? The issue here is that there is the "strictfp" attribute and the llvm::Attribute::StrictFP. We could replace FuncAttrs.addAttribute("strictfp"); with
FuncAttrs.addAttribute(llvm::Attribute::StrictFP);
This function ensures that the function attribute is set when the FunctionDecl attribute is set. I am concerned that when it's removed, we will wind up with cases where the function attribute is missing! The only place where this function attribute is in CodeGenFunction::StartFunction. Is that enough? @andrew.w.kaylor Can you please weigh in on this?

[arsenm]

clang/lib/CodeGen/CodeGenModule.cpp

2135

I currently don't have evidence that making this use the correct attribute would fix anything. If something was depending on this emission in this context, it's already broken

[andrew.w.kaylor]

clang/lib/CodeGen/CodeGenModule.cpp

2135

It may be that anything depending on this is already broken, but the code was written for a reason, even if it was always broken. I'm not sure I understand what that reason was, and unfortunately the person who wrote the code (@mibintc) is no longer actively contributing to LLVM. It was added here: https://reviews.llvm.org/D87528

It does seem like the llvm::Attribute::StrictFP is being added any time the string attribute is added, but they're coming from different places. The proper attribute seems to be coming from CodeGenFunction::StartFunction() which is effectively copying it from the function declaration. It's not clear to me whether there are circumstances where we get to setLLVMFunctionFEnvAttributes() through EmitGlobalFunctionDefinition() without ever having gone through CodeGenFunction::StartFunction(). It looks like maybe there are multiversioning cases that do that, but I couldn't come up with an example that does. @erichkeane wrote a lot of the multi-versioning code, so he might know more, but he's on vacation through the end of the month.

Eliminating this extra string attribute seems obviously good. In this particular location, though, I'd be inclined to set the enumerated attribute here, even though that might be redundant in most cases. On the other hand, if one of our front end experts can look at this code and say definitively that it's always redundant, I'd be fine with this code being deleted.

[arsenm]

clang/lib/CodeGen/CodeGenModule.cpp

2135

I think code that can be deleted that doesn't manifest in test failures should be immediately deleted. We shouldn't leave things around just in case

[kpn]

clang/lib/CodeGen/CodeGenModule.cpp

2135

The Verifier changes that would detect the error and fail tests never made it into the tree. The lack of failures therefore tells us nothing in this case here.

[arsenm]

clang/lib/CodeGen/CodeGenModule.cpp

2135

The verifier never would have checked the string form

[kpn]

clang/lib/CodeGen/CodeGenModule.cpp

2135

Ah, true statement.

[arsenm]

ping

[arsenm]

ping

[zahiraam]

According to this comment https://reviews.llvm.org/D87528#2342132, it looks like @rjmccall had proposed the addition of the attribute. @rjmccall Do you recall why it was added?

[rjmccall]

In D139629#4481030, @zahiraam wrote:

According to this comment https://reviews.llvm.org/D87528#2342132, it looks like @rjmccall had proposed the addition of the attribute. @rjmccall Do you recall why it was added?

My suggestion was that we track the pragma in the Clang AST with an attribute instead of a bit on the Decl. I have no idea why IR generation is adding both a string attribute and the builtin attribute to the IR function, but if doesn't have any obvious effect, I suggest just removing it.

[zahiraam]

In D139629#4481408, @rjmccall wrote:

In D139629#4481030, @zahiraam wrote:

According to this comment https://reviews.llvm.org/D87528#2342132, it looks like @rjmccall had proposed the addition of the attribute. @rjmccall Do you recall why it was added?

My suggestion was that we track the pragma in the Clang AST with an attribute instead of a bit on the Decl. I have no idea why IR generation is adding both a string attribute and the builtin attribute to the IR function, but if doesn't have any obvious effect, I suggest just removing it.

Thanks for your response. I second removing it then.

[arsenm]

self accept after latest comments

[arsenm]

42d4c85ca83f25f993444fb5bbaa58525f724991

clang/lib/CodeGen/CodeGenModule.cpp

for (const CXXRecordDecl *Base : getMostBaseClasses(MD->getParent())) {
       llvm::Metadata *Id =
           CreateMetadataIdentifierForType(Context.getMemberPointerType(
               MD->getType(), Context.getRecordType(Base).getTypePtr()));
       F->addTypeMetadata(0, Id);
     }
   }
 }
 
-void CodeGenModule::setLLVMFunctionFEnvAttributes(const FunctionDecl *D,
-                                                  llvm::Function *F) {
-  if (D->hasAttr<StrictFPAttr>()) {
-    llvm::AttrBuilder FuncAttrs(F->getContext());
-    FuncAttrs.addAttribute("strictfp");
-    F->addFnAttrs(FuncAttrs);
-  }
-}
-
 void CodeGenModule::SetCommonAttributes(GlobalDecl GD, llvm::GlobalValue *GV) {
   const Decl *D = GD.getDecl();
   if (isa_and_nonnull<NamedDecl>(D))
     setGVProperties(GV, GD);
   else
     GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 
   if (D && D->hasAttr<UsedAttr>())
void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
 
   // FIXME: this is redundant with part of setFunctionDefinitionAttributes
   setGVProperties(Fn, GD);
 
   MaybeHandleStaticInExternC(D, Fn);
 
   maybeSetTrivialComdat(*D, *Fn);
 
-  // Set CodeGen attributes that represent floating point environment.
-  setLLVMFunctionFEnvAttributes(D, Fn);
-
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);
 
   setNonAliasAttributes(GD, Fn);
   SetLLVMFunctionAttributesForDefinition(D, Fn);
 
   if (const ConstructorAttr *CA = D->getAttr<ConstructorAttr>())
     AddGlobalCtor(Fn, CA->getPriority());
   if (const DestructorAttr *DA = D->getAttr<DestructorAttr>())

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

MyComplex useAdd() {
    return a + b;
 }
 
 // CHECK-DDEFAULT: Function Attrs: noinline nounwind{{$$}}
 // CHECK-DEBSTRICT: Function Attrs: noinline nounwind strictfp{{$$}}
 // CHECK-FAST: Function Attrs: noinline nounwind{{$$}}
 // CHECK-NOHONOR: Function Attrs: noinline nounwind{{$$}}
 // CHECK-LABEL: define{{.*}} @_GLOBAL__sub_I_fp_floatcontrol_stack
+
+// CHECK-DEBSTRICT: {{[ ]}}strictfp{{[ ]}}
+// CHECK-DEBSTRICT-NOT: "strictfp"