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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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)
----------------
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?


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


================
Comment at: llvm/test/CodeGen/PowerPC/fma-precision.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -verify-machineinstrs -mcpu=pwr9 -mtriple=powerpc64le-linux-gnu | FileCheck %s
+; RUN: llc < %s -fp-contract=fast -verify-machineinstrs -mcpu=pwr9 -mtriple=powerpc64le-linux-gnu | FileCheck %s
 
----------------
Similar to the earlier inline comment (see D99080 for details). Can we avoid using the global flag by updating FMF instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104247