Page MenuHomePhabricator

[clang][patch][FPEnv] Make Initialization of C++ globals strictfp aware

Authored by mibintc on May 12 2021, 11:08 AM.



In the proposed patch @kpn pointed out that the global variable initialization functions didn't have the "strictfp" metadata set correctly, and @rjmccall said that there was buggy code in SetFPModel and StartFunction, this patch is to solve those problems. When Sema creates a FunctionDecl, it sets the FunctionDeclBits.UsesFPIntrin to "true" if the lexical FP settings (i.e. a combination of command line options and #pragma float_control settings) correspond to ConstrainedFP mode. That bit is used when CodeGen starts codegen for a llvm function, and it translates into the "strictfp" function attribute.

Diff Detail

Event Timeline

mibintc created this revision.May 12 2021, 11:08 AM
mibintc requested review of this revision.May 12 2021, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 11:08 AM
mibintc added inline comments.May 12 2021, 11:13 AM

there's a whole bunch of places in this func that creates FD, I changed them into a single place. hope that's OK, or i could pull it out as a pre-patch if you prefer

aaron.ballman added inline comments.

Formatting looks off here.


If ctors and cxxmethods get the parameter, why not conversion operators?


Should lambdas pick up the same fp constraints as the function defining the lambda?

mibintc updated this revision to Diff 359772.Jul 19 2021, 6:38 AM
mibintc added a reviewer: zahiraam.

Respond to @aaron.ballman 's review, rebased & used clang-format

mibintc marked an inline comment as done.Jul 19 2021, 6:41 AM
mibintc added inline comments.

I made it use the FP settings from Sema,

aaron.ballman added inline comments.Jul 20 2021, 5:39 AM

I have no idea if others agree, but I have a slight preference for putting UsesFPIntrin towards the end of the parameter list (here and in the Create methods) -- this flag seems less important than the constexpr kind or whether inline is specified (and, it helpfully avoids putting two bool parameters next to one another).

If you agree, perhaps it could be moved to before TrailingRequiresClause?


I think it'd be better to separate these changes into an NFC patch (feel free to land it without review; I've checked the changes here and they look correct and NFC).


Please spell out the type rather than use auto, also, should probably be FP rather than fp per the usual naming conventions.

mibintc updated this revision to Diff 360222.Jul 20 2021, 11:50 AM

Partially respond to @aaron.ballman 's review by refactoring a change into a separate commit, but I'll push back on another request, I'll add that reply inline.

mibintc marked an inline comment as done.Jul 20 2021, 11:55 AM

Question for Aaron


I spent a few hours recoding to rearrange the parameter list, but there are downstream types from FunctionDecl which would presumably also need to be rearranged, and those types add more parameters and some of those parameters have default values, and moving the new boolean before TrailingRequiresClause doesn't work, I decided to throw away the mess I made and ask if you could live with it this way.


oops i didn't get this one

aaron.ballman added inline comments.Jul 20 2021, 12:02 PM

Yup, I can live with it this way if changing it causes trouble. Thanks!

mibintc updated this revision to Diff 360225.Jul 20 2021, 12:06 PM

Removed use of auto, and used different capitalization for local var name

The changes LGTM, but I'd appreciate additional sign-off from someone with more expertise in this area. @rjmccall, can you take a look to see if it addresses your concerns?

martong removed a subscriber: martong.Jul 28 2021, 9:20 AM
aaron.ballman accepted this revision.Jul 29 2021, 5:40 AM

Giving my official LGTM now that there's been a week for folks to comment with additional concerns.

This revision is now accepted and ready to land.Jul 29 2021, 5:40 AM
This revision was landed with ongoing or failed builds.Jul 29 2021, 9:03 AM
This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [clang][patch][FPEnv} Initialization of C++ globals not strictfp aware to [clang][patch][FPEnv] Make Initialization of C++ globals strictfp aware.Jul 29 2021, 10:04 AM
MaskRay added a subscriber: MaskRay.EditedJul 29 2021, 10:08 AM

If you upload a diff via "Upload Diff" on webUI or arc diff 'HEAD^', by default the subject/summary are not updated.
(arc diff --verbatim 'HEAD^' updates them.)

After editing your local commit message, please update the review as well to be synchronized.

The patch changed the signature of FunctionDecl::Create. When changing clang/include/clang headers, it's a good idea to test check-lldb check-clang-tools (-DLLVM_ENABLE_PROJECTS='...;clang-tools-extra;lldb' in case clang-tools-extra/ and lldb/ have code needing updates.
In this case there was an lldb build failure I just fixed. I assume that passing false (/*UsesFPIntrin=*/false) is safe.

@MaskRay Thanks a lot -- yes!

MaskRay added inline comments.Jul 29 2021, 11:13 AM

canonical form is /*XXX=*/YYY


Missing colon


Missing colon?


You need {{$}} to check there is no strictfp at the end.