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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
