[PATCH] D136786: Fix `unsafe-fp-math` attribute emission.

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

In D136786#3893485 <https://reviews.llvm.org/D136786#3893485>, @andrew.w.kaylor wrote:

> As a first principle, I'd like to clarify a "big picture" item that I was trying to get at in my earlier comment. I'd like to be able to reason about this from the starting point of individual semantic modes. There is a set of modes defined in the clang user's manual (https://clang.llvm.org/docs/UsersManual.html#controlling-floating-point-behavior). I think this is reasonably complete:
>
>   exception_behavior
>   fenv_access
>   rounding_mode
>   fp-contract
>   denormal_fp_math
>   denormal_fp_math_32
>   support_math_errno
>   no_honor_nans
>   no_honor_infinities
>   no_signed_zeros
>   allow_reciprocal
>   allow_approximate_fns
>   allow_reassociation
>
> These are the modes from clang's perspective. They get communicated to the back end in different ways. The backend handling of this isn't nearly as consistent as I'd like, but that's a different problem. I think these basic modes can be thought of as language-independent and should be used as the basic building blocks for reasoning about floating point behavior across all LLVM-based compilers.
>
> On top of these modes, we have two concepts "fast-math" and "unsafe-math-optimizations" which are essentially composites of one or more of the above semantic modes. I say "essentially" because up to this point there has been some fuzzy handling of that. I'd like to say they are absolutely composites of the above modes, and I think we can make it so, starting here.
>
> If we agree that fast-math and unsafe-math-optimizations/unsafe-fp-math are composites of some of the other modes, then we can apply a symmetry property that I think will be helpful in the problem we're trying to solve here and will lead to better reasoning about these settings in the future.
>
> I am proposing that passing "-ffast-math" should have *exactly* the same effects as passing all of the individual command line options that are implied by -ffast-math. Likewise, passing "-funsafe-math-optimizations" should have *exactly* the same effects as passing all of the individual options that are implied by -funsafe-math-optimizations. And, of course, any combination of options that turn various states on and off can be strung together and we can just evaluate the final state those options leave us in to see if we are in the "fast-math" state or the "unsafe-fp-math" state.

Yes, I agree that this should be the long term goal.

> I'm going to ignore fast-math right now, because I think the current handling is mostly OK, and this review is about unsafe-fp-math. The unsafe-fp-math case is a little simpler in concept, but I think we've got more problems with it.
>
> Another fundamental principle I'd like to declare here is that LangOpts.UnsafeFPMath, TargetOptions.UnsafeFPMath, and the "unsafe-fp-math" function attribute should all mean exactly the same thing. The function attribute has a different scope, so it might not always have the same value as the other two, but it should at least mean the same thing.
>
> My understanding is this:
>
>   unsafe-fp-math = exception_behavior(ignore) +
>                    fenv_access(off) +
>                    no_signed_zeros(on) +
>                    allow_reciprocal(on) +
>                    allow_approximate_fns(on) +
>                    allow_reassociation(on) +
>                    fp_contract(fast)
>
> The first two of those settings are the default behavior. The others can be turned on by individual command line options. If all of those semantic modes are in the states above, then we are in "unsafe-fp-math" mode. That's my proposal.
>
> There are a couple of things we need to talk about here:
>
> 1. Currently, the Driver/ToolChains/Clang.cpp RenderFloatingPointOptions() function is looking for !MathErrno before deciding to pass "-funsafe-math-optimizations" to the front end, so LangOpts.UnsafeFPMath will not be set unless math-errno support is off. However, the RenderFloatingPointOptions() handling of -funsafe-math-optimizations does not change the MathErrno setting. Clearly, there's a bug here one way or the other. In GCC -funsafe-math-optimizations does not affect the math errno handling, so I think that's the correct behavior.

I agree there is a bug. My resolution for this would be to remove the `!MathErrno` from:

  if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
      ApproxFunc && !TrappingMath)
    CmdArgs.push_back("-funsafe-math-optimizations");

so that we match GCC behavior and definition.

> 2. Currently, the Driver/ToolChains/Clang.cpp RenderFloatingPointOptions() function does not consider the FPContract setting when deciding whether to pass "-funsafe-math-optimizations" to the front end or set the fp-contract value when handling the -funsafe-math-optimizations option. However, there are places in the backend that interpret the "unsafe-fp-math" function attribute (and I think also the TargetOptions.UnsafeFPMath flag) as allowing FMA exactly as fp-contract(fast) would. I think this is a case where for some reason the meaning of the "unsafe-fp-math" function attribute has diverged from the meaning of the -funsafe-math-optimizations option, but I can't think of any reason that the -funsafe-math-optimizations option should not allow fast fp-contract, so I think we should make it do so. This option derives from gcc, but gcc doesn't connect the fp-contract behavior to the fast-math options at all, and frankly, I think their fp-contract handling is fairly shoddy, so I think we should diverge from them on this point.

The fact that `"unsafe-fp-math"="true"` implies `-ffp-contract=fast` is quite unfortunate, and it is something that in principle should be addressed.
My understanding is that the floating point math function attributes are a quite old concept that predates the per-instruction fast-math flags. Ideally we should get rid of the flooding point math function attribute since we do have a finer grain representation.
A while back the main issue was that all the backends code (e.g. DAGCombiner) were only using the `TargetOptions` properties (hence the function attributes) to control the floating point related transformations, hence there would have been regressions.

IMO being more conservative w.r.t. conditions for which the frontend emits the `"unsafe-fp-math"` attribute is a small step in the direction of phasing out the floating point math function attributes. However I don't know if the backend code has been enhanced to consider the per instruction fast-math flags to control transformations. If the backend code generators are now fast-math-flags aware, then there shouldn't be any significant regression due to missing *legal* transformations.

At high level, I think that we need to understand how important is to match GCC behavior in this particular case. We can change the way Clang defines `-funsafe-math-optimizations` to imply `-ffp-contract=fast`, but that seems the wrong solution as it feels like promoting a bug to a feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786