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

Mon Oct 31 07:04:11 PDT 2022

zahiraam added a comment.

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

> In D136786#3892177 <https://reviews.llvm.org/D136786#3892177>, @zahiraam wrote:
>
>>>> I'm not following entirely, but -funsafe-math-optimizations is just a subset of -ffast-math, so checking only the properties that contribute to -funsafe-math-optimizations should be enough. 
>>>>  I think it is way simpler to reason in these terms than enumerating all the possible scenarios.
>>
>> Exactly my point Since -funsafe-math-optimizations is a subset of -ffast-math, shouldn't it be the "minimal" option to trigger the unsafe-fp-math attribute for functions? 
>> I agree with the second part of the condition; it should be
>> updated with (LangOpts.getDefaultFPContractMode() ==  LangOptions::FPModeKind::FPM_Fast ||   LangOpts.getDefaultFPContractMode() == LangOptions::FPModeKind::FPM_FastHonorPragmas) 
>> but I am still hesitant about the change you are proposing for the first part of the condition.
>> @andrew.w.kaylor Can you please weigh in on this?
>
> I talked to Zahira about this offline, and the current state is even messier than I realized. I think we need to start by making sure we agree on the behavior we want and then figure out how to get there. There are some messy complications in the different ways things are handled in the driver, the front end, and the backend. There are more overlapping settings than I would like, but I guess it's best to look for the best way we can incrementally improve that situation.
>
> 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.

Strongly agree.

> 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.

Agree. This is not currently the case.  -funsafe-math-optimizations should set the FPContract=fast the same way it's done for -ffast-math. I think it makes sense to fix this in this patch. @michele.scandale WDYT?

> 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.

Yep!   This can be fixed in an upcoming patch. @michele.scandale Do you want to do it? If not, I can.

> 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.

If we stick to the meaning of unsafe math calculations as being what's defined above, then it makes sense to me that unsafe math calculation mode implies FPContract=fast.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786