In fast-math mode, when unsafe math optimizations are enabled, the compiler is allowed to use optimizations that allow reassociation and transformations that don’t guaranty accuracy.
For example (x+y)+z is transformed into x+(y+z) . Although mathematically equivalent, these two expressions may not lead to the same final result due to errors of summation.
Or x/x is transformed into 1.0 but x could be 0.0, INF or NaN. And so this transformation also may not lead to the same final result.
Setting the eval method 'ffp-eval-method' or via '#pragma clang fp eval_method' in this mode, doesn’t have any effect.
This patch adds code to warn the user of this.
Details
Diff Detail
Event Timeline
clang/include/clang/Basic/DiagnosticCommonKinds.td | ||
---|---|---|
127 ↗ | (On Diff #416990) | |
128–129 ↗ | (On Diff #416990) | FWIW, I'm still not quite certain what the diagnostic is telling me. I don't know what "value-safe" means in this context, so we may need to tweak this further. |
clang/lib/Parse/ParseStmt.cpp | ||
1065 ↗ | (On Diff #416990) | This looks like it's going to diagnose on every compound statement regardless of whether the function contains any floating-point operations at all. If so, that's way too chatty. It seems like the issue here is "the command line options disagree", so why is this not a frontend warning when processing the command line options? |
clang/test/Sema/eval-method-with-unsafe-math.c | ||
38 | Once we issue the warning for the pragma, we should not issue the warning a second time for the statement. |
clang/lib/Parse/ParseStmt.cpp | ||
---|---|---|
1065 ↗ | (On Diff #416990) | I tried to do that initially but I don't have a Location for the diagnostic. |
clang/lib/Parse/ParseStmt.cpp | ||
---|---|---|
1065 ↗ | (On Diff #416990) | We issue plenty of diagnostics when command line options don't match expectations, as examples: https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L525 (and so on). |
clang/include/clang/Basic/DiagnosticCommonKinds.td | ||
---|---|---|
127–129 ↗ | (On Diff #417279) | I think we should split the diagnostics into two -- one as a Sema diagnostic (as a warning for the pragma case, in the Pragmas diagnostic group) and one as a Frontend diagnostic (as an error, for the command line case). Basically, I think we should treat the command line options as being mutually exclusive and handle it the same as any other "these options cannot be used together" situation. |
clang/lib/Sema/Sema.cpp | ||
259–260 ↗ | (On Diff #417279) | I don't think this is the right place to diagnose this -- these are command line options that don't play well together, so they should be handled when processing command line options. |
clang/lib/Sema/SemaAttr.cpp | ||
---|---|---|
492 | Not sure if I should repeat the comment here (same one than in CompilerInvocation.cpp?). |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 | Unless you have a strong reason for this to be a warning, this seems like a situation we should diagnose as an error with a much clearer message. | |
clang/include/clang/Basic/DiagnosticGroups.td | ||
127 ↗ | (On Diff #417356) | This change won't be needed any longer. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
6495–6497 | Similar wording suggestion here as above -- this makes it more clear what the issue is and how the user can fix it. You'll have to figure out which pragmas and which command line options set the language options you're checking for the predicate. But the goal is to let users know where the conflict is. Bonus points if you can add a note to point to the previous pragma that caused the conflict with clang fp eval_method so the user doesn't have to hunt for it. However, I don't insist on that as it may be difficult to track. But if it isn't difficult, it would allow us to improve the %select slightly to something like: "'#pragma clang fp eval_method' cannot be used with %select{'#pragma clang fp reassociate'|'-mreassociate'|'#pragma whatever'|'-fwhatever'|..}0">, | |
clang/lib/Sema/SemaAttr.cpp | ||
492 | Er, given that they're pretty involved comments, I'd only put the comments in one place, and then have the other one say "See the comments in <some reasonable identifying location> for more information about why this is diagnosed." or something to that effect. |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 | May be @andrew.w.kaylor would weigh in on this? |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 | I was going to say that for the command line option we could just issue a warning saying that the later option overrides the earlier, but it's a bit complicated to sort out what that would mean if the eval method follows a fast-math option and it might not always be what the user intended. So, I guess I'd agree that it should be an error. For the case with pragmas, the model I'd follow is the mixing of #pragma float_control(except, on) with a fast-math mode or #pragma float_control(precise, off) with a non-ignore exception mode. In both those cases we issue an error. |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 |
Good catch, I think that's a good approach as well. |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 | I think i will have the issue with the order of appearance of the options on the command line. will depend on which order I will test for LangOpts.ApproxFunc/AllowFPReasson/AllowRecip being used or not? The run lines above might give the same diagnostic. Unless I do something really complicated to check the order of the options on the command line? |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 |
You shouldn't -- you should be able to test the language options after the command line was fully parsed. See FixupInvocation() in CompilerInvocation.cpp. |
clang/include/clang/Basic/DiagnosticFrontendKinds.td | ||
---|---|---|
50–53 | I still prefer the suggested wording I had originally: "'-ffp-eval-method' cannot be used with '%0'"; I think it's a good generalization but still sufficiently informative. WDYT? | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
6496 | I'm not certain what pragma_clang_fp_eval_reassociate is? | |
6497–6505 | These should be combined into one diagnostic, which I believe we wanted to be an error instead of a warning. Also, because this will trigger for pragma use OR command line argument use, I think we need to be more generalize about what it cannot be used with. e.g., '#pragma clang fp eval_method' cannot be used when %select{approximate functions|reassociation|reciprocal whatever}0 is enabled or something along those lines (I'm hoping @andrew.w.kaylor can help figure out what the best terminology is here, as I'm not super familiar with those floating-point features). |
Unless you have a strong reason for this to be a warning, this seems like a situation we should diagnose as an error with a much clearer message.