In the proposed patch https://reviews.llvm.org/D81178 @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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
930 ms | x64 debian > libomp.lock::omp_init_lock.c |
Event Timeline
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
700 | 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 |
clang/lib/Sema/SemaLambda.cpp | ||
---|---|---|
395 | I made it use the FP settings from Sema, |
clang/include/clang/AST/Decl.h | ||
---|---|---|
1964–1965 | 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? | |
2569 | ||
2573 | ||
clang/lib/CodeGen/CodeGenFunction.cpp | ||
700 | 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). | |
928 | Please spell out the type rather than use auto, also, should probably be FP rather than fp per the usual naming conventions. |
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.
Question for Aaron
clang/include/clang/AST/Decl.h | ||
---|---|---|
1964–1965 | 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. | |
clang/lib/CodeGen/CodeGenFunction.cpp | ||
928 | oops i didn't get this one |
clang/include/clang/AST/Decl.h | ||
---|---|---|
1964–1965 | Yup, I can live with it this way if changing it causes trouble. Thanks! |
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?
Giving my official LGTM now that there's been a week for folks to comment with additional concerns.
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.
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?