This is an archive of the discontinued LLVM Phabricator instance.

clang: Stop emitting "strictfp"
ClosedPublic

Authored by arsenm on Dec 8 2022, 6:32 AM.

Details

Summary

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

Diff Detail

Event Timeline

arsenm created this revision.Dec 8 2022, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 6:32 AM
arsenm requested review of this revision.Dec 8 2022, 6:32 AM
kpn added a comment.Dec 8 2022, 8:57 AM

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.

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?

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

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 added inline comments.Dec 8 2022, 1:54 PM
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 added inline comments.Dec 13 2022, 3:26 PM
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 added inline comments.
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 added inline comments.Dec 14 2022, 2:23 PM
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 added inline comments.Dec 19 2022, 4:52 AM
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 added inline comments.Dec 19 2022, 6:36 AM
clang/lib/CodeGen/CodeGenModule.cpp
2135

The verifier never would have checked the string form

kpn added inline comments.Dec 19 2022, 7:24 AM
clang/lib/CodeGen/CodeGenModule.cpp
2135

Ah, true statement.

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?

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.

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 accepted this revision.Jul 7 2023, 12:27 PM

self accept after latest comments

This revision is now accepted and ready to land.Jul 7 2023, 12:27 PM