[PATCH] D93891: [DAGCombine] Remove the check for unsafe-fp-math when we are checking the AFN

Wed Dec 30 06:08:08 PST 2020

spatel added reviewers: aemerson, craig.topper, fhahn.
spatel added a comment.

In D93891#2474602 <https://reviews.llvm.org/D93891#2474602>, @steven.zhang wrote:

> In D93891#2473825 <https://reviews.llvm.org/D93891#2473825>, @spatel wrote:
>
>> We're trying to (very slowly...) move away from needing the global/target options. Also, I'm not sure how those translate with GlobalISel.
>> If there's a real-world case where the target options do not match the fast-math-flags on the IR, we should investigate if that was unexpected. Otherwise, I would prefer that we not add to the uses of target options.
>
> Hmm, do you know the specific reason that it is removed so slowly ... (What I imagine is just removing it and update some tests. Some targets still want it ?) It is not right to me that it is handled inconsistent. i.e.
> FPOW didn't check the unsafe-fp-math while SQRT check it.

Some targets had concerns about relying on only the node-level FMF, but I don't remember exactly why. It's been a long time since we introduced that functionality, so maybe it's fine to update now. 
@arsenm - thoughts? (also adding more people who might answer if/how FMF changes affect GlobalIsel)

> As the direction is to remove this option, I tried to remove the unsafe-fp-math for sqrt to make the check for afn consistent in LLVM. I could do more but I am afraid I miss some background that stop this removal ...

I agree that we should be consistent, so I agree with the updated patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93891