[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

Mon Sep 9 08:39:49 PDT 2019

mibintc marked 3 inline comments as done.
mibintc added inline comments.

================
Comment at: clang/docs/UsersManual.rst:1305
+   and ``noexcept``. Note that -fp-model=[no]except can be combined with the
+   other three settings for this option. Details:
+
----------------
andrew.w.kaylor wrote:
> rjmccall wrote:
> > mibintc wrote:
> > > rjmccall wrote:
> > > > Combined how?  With a comma?
> > > > 
> > > > This option seems to have two independent dimensions.  Is that necessary for command-line compatibility with ICC, or can we separate it into two options?
> > > > 
> > > > The documentation should mention the default behavior along both dimensions.  Is it possible to override a prior instance of this option to get this default behavior back?
> > > > 
> > > > You mention that this `-fp-model=fast` is equivalent to `-ffast-math`.  How does this option interact with that one if both are given on a command line?
> > > > 
> > > > Please put option text in backticks wherever it appears.
> > > > 
> > > > Most of these comments apply to `-fp-speculation` as well.
> > > > Combined how? With a comma?
> > > 
> > > > This option seems to have two independent dimensions. Is that necessary for command-line compatibility with ICC, or can we separate it into two options?
> > > Yes that's right, there are 2 dimensions.  I wrote it like this for identical compatibility with icc, and cl.exe also defines the option this way, to specify multiple values simultaneously. However I think it would be reasonable and good to split them into separate options.  I will discuss this with the folks back home.
> > > 
> > > > The documentation should mention the default behavior along both dimensions. 
> > > I added this info into the doc
> > > 
> > > > Is it possible to override a prior instance of this option to get this default behavior back?
> > > The 3 values along one dimension, precise, strict, fast if they appear multiple times in the command line, the last value will be the setting along that dimension.  Ditto with the other dimension, the rightmost occurrence of except or noexcept will be the setting. 
> > > 
> > > > You mention that this -fp-model=fast is equivalent to -ffast-math. How does this option interact with that one if both are given on a command line?
> > > The idea is that they are synonyms so if either or both appeared on the command line, the effect would be identical. 
> > > 
> > > I'll upload another patch with a few documentation updates and get back to you about splitting the fp-model option into multiple options.  (Longer term, there are 2 other dimensions to fp-model)
> > > 
> > > And thanks for the review
> > > Yes that's right, there are 2 dimensions. I wrote it like this for identical compatibility with icc, and cl.exe also defines the option this way, to specify multiple values simultaneously. However I think it would be reasonable and good to split them into separate options. I will discuss this with the folks back home.
> > 
> > Okay.  There's certainly some value in imitating existing compilers, but it sounds like a lot has been forced into one option, so maybe we should take the opportunity to split it up.  If we do split it, though, I think the different dimensions should have different base spellings, rather than being repeated uses of `-fp-model`.
> > 
> > > The 3 values along one dimension, precise, strict, fast if they appear multiple times in the command line, the last value will be the setting along that dimension.
> > 
> > Okay.  This wasn't clear to me from the code, since the code also has an "off" option.
> > 
> > > The idea is that they are synonyms so if either or both appeared on the command line, the effect would be identical.
> > 
> > Right, but compiler options are allowed to conflict with each other, with the general rule being that the last option "wins".  So what I'm asking is if that works correctly with this option and `-ffast-math`, so that e.g. `-ffast-math -fp-model=strict` leaves you with strict FP but `-fp-model=strict -ffast-math` leaves you with fast FP.  (That is another reason why it's best to have one aspect settled in each option: because you don't have to merge information from different uses of the option.)
> > 
> > At any rate, the documentation should be clear about how this interacts with `-ffast-math`.  You might even consider merging this into the documentation for `-ffast-math`, or at least revising that option's documentation.  Does `-fp-model=fast` cause `__FAST_MATH__` to be defined?
> > 
> > Also, strictly speaking, this should be `-ffp-model`, right?
> I think the ICC interface includes the exception option for compatibility/consistency with Microsoft's  /fp option. We can handle that in clang-cl. So, I agree that it makes sense to split that out in clang.
> 
> ICC's implementation of this actually has four dimensions, only two of which are being taken on here. Frankly, I think it's a bit of a mess. The core concept which I think we should bring into clang with this option is to have a single option that manages all the various settings to control floating point behavior to produce the primary expected modes of operation so users don't have to find all the flags and remember the default settings for each one.
> 
> The way I'd suggest this should work is that we provide just the  primary "models" and allow other options to modify the base behavior, regardless of the order in which the options appear. So, for example, 
> 
> -fp-model=precise -fp-speculation=safe
> 
> and
> 
> -fp-speculation=stafe -fp-model=precise
> 
> would both mean the same thing, disable value-unsafe optimizations and prevent speculative execution of floating point operations. I don't know how painful that is from a driver perspective or how obvious it would be to "most users" but to me it seems to be the logical result of fp-model being an umbrella setting and other options being able to modify it.
Thanks for the review. I'm going to upload anotoher patch which drops -fp-model=[no-]except.  This will clean up the command line for the fp-model setting because now it cannot have 2 settings simultaneously.  The new patch will drop the fp-speculation option, and add a new option fp-exception-behavior. The fp-exception-behavior option allows access to the "eb" exception behavior setting of the LLVM constrained floating point intrinsics.  The patch is pseudo code at this point because I want to get @rjmccall response to this proposal before finalizing.  Since fp-model is an umbrella option, there are conflicts between it and existing options. I added pseudo code into RenderFloatingPointOptions to detect and report the conflicts, and rewrote the part that detects inter-option conflicts. 

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62731/new/

https://reviews.llvm.org/D62731