[PATCH] D104247: [DAGCombine] reassoc flag shouldn't enable contract

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

LGTM.
Since we are opting for a definition of `reassoc` that is not a superset of the other flags, it might be worth seeing if transforms related to `arcp` and `afn` are similarly affected.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13257
   if (Aggressive) {
+    bool CanFuse = Options.UnsafeFPMath || N->getFlags().hasAllowContract();
     // fold (fsub (fma x, y, (fmul u, v)), z)
----------------
jsji wrote:
> jsji wrote:
> > spatel wrote:
> > > If we still allow the global/deprecated UnsafeFPMath option to enable FMA formation, we are keeping a side-channel way to override the FMF. That is, the global setting is effectively a replacement for "reassoc" in (many?) regression tests.
> > > 
> > > I think this is not changing the logic before this patch, so that's fine, but do you plan to fix that too?
> > Yes, I planned to fix this -- by allowing it for global flag as well , not only `UnsafeFPMath`.
> > 
> > 
> > 
> Oh, just to double confirm, we will still allow global options to override FMF (UnSafeFPMath, -ffast-math, etc), right? FMF is great, but sometimes global options will be more handy for front-end or IR producers. It may take time for IR producers to transit to FMF only?
The goal of IR- and node-level FMF is to obsolete/deprecate the global options. But this has been the case for many years now, so I have no guess for how much longer it will take. :)


================
Comment at: llvm/test/CodeGen/PowerPC/fma-assoc.ll:323
 
+; FIXME: -ffp-contract=fast does NOT work here?
 define double @test_reassoc_FMSUB_ASSOC1(double %A, double %B, double %C,
----------------
jsji wrote:
> spatel wrote:
> > Ideally, we would remove the RUN lines that have that setting and rely on IR-level FMF instead. This may require duplicating some tests and updating the FMF.
> > 
> > We are trying to rely on the IR and node-level alone if possible - see D99080 for discussion.
> Can we do this in another follow up patch? As you mentioned in D99080, I believe there are quite some tests needs updates.
Sure - this is going to require manual inspection, adding tests, etc. I just wanted to point out that fixing the global option might not be worth the effort (if the code is behaving as expected using FMF).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104247