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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
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)
----------------
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`.





================
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:
> 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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104247