aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:50-53
+def warn_eval_method_setting_via_option_in_value_unsafe_context : Warning<
+ "setting the eval method via '-ffp-eval-method' has not effect when
numeric "
+ "results of floating-point calculations aren't value-safe.">,
+ InGroup<IncompatibleFPOpts>;
----------------
aaron.ballman wrote:
> zahiraam wrote:
> > aaron.ballman wrote:
> > > andrew.w.kaylor wrote:
> > > > zahiraam wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > May be @andrew.w.kaylor would weigh in on this?
> > > > 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.
> > > > 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.
> > 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?
> > 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`.
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?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6480
+ "eval method setting via '%0' cannot be used with "
+ "'pragma_clang_fp_eval_reassociate'">, InGroup<Pragmas>;
+def warn_pragma_clang_fp_eval_method_used_with_fapprox_func : Warning<
----------------
I'm not certain what `pragma_clang_fp_eval_reassociate` is?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6481-6489
+def warn_pragma_clang_fp_eval_method_used_with_fapprox_func : Warning<
+ "'#pragma clang fp eval_method' cannot be used with 'fapprox_func'">,
+ InGroup<Pragmas>;
+def warn_pragma_clang_fp_eval_method_used_with_mreassociate : Warning<
+ "'#pragma clang fp eval_method' cannot be used with 'mreassociate'">,
+ InGroup<Pragmas>;
+def warn_pragma_clang_fp_eval_method_used_with_freciprocal : Warning<
----------------
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).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122155/new/
https://reviews.llvm.org/D122155
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
