This is an archive of the discontinued LLVM Phabricator instance.

Add warning when eval-method is set in the presence of value unsafe floating-point calculations.
ClosedPublic

Authored by zahiraam on Mar 21 2022, 9:34 AM.

Details

Summary

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.

Diff Detail

Event Timeline

zahiraam created this revision.Mar 21 2022, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 9:34 AM
zahiraam requested review of this revision.Mar 21 2022, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 9:34 AM
aaron.ballman added inline comments.Mar 21 2022, 11:49 AM
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.

zahiraam added inline comments.Mar 21 2022, 12:25 PM
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.

aaron.ballman added inline comments.Mar 21 2022, 12:32 PM
clang/lib/Parse/ParseStmt.cpp
1065 ↗(On Diff #416990)
zahiraam updated this revision to Diff 417279.Mar 22 2022, 6:45 AM
zahiraam edited the summary of this revision. (Show Details)
zahiraam marked 5 inline comments as done.
aaron.ballman requested changes to this revision.Mar 22 2022, 7:13 AM
aaron.ballman added inline comments.
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.

This revision now requires changes to proceed.Mar 22 2022, 7:13 AM
zahiraam updated this revision to Diff 417356.Mar 22 2022, 11:52 AM
zahiraam marked 2 inline comments as done.
zahiraam added inline comments.
clang/lib/Sema/SemaAttr.cpp
492

Not sure if I should repeat the comment here (same one than in CompilerInvocation.cpp?).

aaron.ballman added inline comments.Mar 22 2022, 12:19 PM
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.

zahiraam added inline comments.Mar 22 2022, 12:27 PM
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.

aaron.ballman added inline comments.Mar 23 2022, 7:24 AM
clang/include/clang/Basic/DiagnosticFrontendKinds.td
50–53

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.

Good catch, I think that's a good approach as well.

zahiraam marked 2 inline comments as done.Mar 23 2022, 1:37 PM
zahiraam added inline comments.
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.
RUN: -freciprocal-math -mreassociate -ffp-eval-method=source
and
RUN: -mreassociate -ffp-eval-method=source

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?

aaron.ballman added inline comments.Mar 24 2022, 5:31 AM
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.

You shouldn't -- you should be able to test the language options after the command line was fully parsed. See FixupInvocation() in CompilerInvocation.cpp.

zahiraam updated this revision to Diff 418210.Mar 25 2022, 6:43 AM
aaron.ballman added inline comments.Mar 25 2022, 6:53 AM
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).

zahiraam updated this revision to Diff 418597.Mar 28 2022, 8:31 AM
zahiraam marked 5 inline comments as done.
aaron.ballman added inline comments.Mar 28 2022, 2:17 PM
clang/include/clang/Basic/DiagnosticFrontendKinds.td
52–53
clang/include/clang/Basic/DiagnosticSemaKinds.td
6495–6497
clang/test/Sema/eval-method-with-unsafe-math.c
53

Can you split the driver tests into their own file in the Driver directory?

zahiraam updated this revision to Diff 418936.Mar 29 2022, 11:22 AM
zahiraam marked 3 inline comments as done.
zahiraam updated this revision to Diff 419746.Apr 1 2022, 6:59 AM
zahiraam updated this revision to Diff 419763.Apr 1 2022, 8:10 AM
zahiraam updated this revision to Diff 420035.Apr 3 2022, 4:47 AM
zahiraam updated this revision to Diff 420168.Apr 4 2022, 6:46 AM
aaron.ballman accepted this revision.Apr 4 2022, 11:43 AM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 4 2022, 11:43 AM