[PATCH] D39304: [IR] redefine 'reassoc' fast-math-flag and add 'trans' fast-math-flag

[Warren Ristow via Phabricator via llvm-commits]

wristow added inline comments.


================
Comment at: include/llvm/IR/Operator.h:220
+  void setApproxFunc()      { Flags |= ApproxFunc; }
+  void setFast()            { set(); }
 
----------------
spatel wrote:
> wristow wrote:
> > One loose end that needs to be taken care of more or less simultaneously is a Clang change.  Specifically, the constructor for `CodeGenFunction` (in "CodeGenFunction.cpp") invokes `FastMathFlags::setUnsafeAlgebra()`, so it will need to be changed to `setFast()`.
> This is correct - I didn't post it, but I have that one line patch in place locally, so I was planning to submit it to the clang repo as close as possible after this patch and reference this commit (if there's a way to avoid the build breakage cleanly, please let me know).
I don't know of a clean way.  Definitely fine with me to submit it right after this patch is submitted.


================
Comment at: include/llvm/IR/Operator.h:187
+    ApproxFunc      = (1 << 6)
   };
 
----------------
We'll need to add flags to `SDNodeFlags` that are analogous to `AllowReassoc` and `ApproxFunc`.  Adding them in a separate patch seems fine, but in case the lack of that change in this patch was an oversight, I wanted to raise the point here.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2018
 
   // Don't optimize floating point instructions that don't have unsafe algebra.
+  if (I->getType()->isFPOrFPVectorTy() && !I->isFast())
----------------
Very minor point/question: Since the test is no longer `hasUnsafeALgebra()`, are we OK with the comment still saying `unsafe algebra`?  Or do we want to change the comment above to something like:

`// Don't optimize floating point instructions that don't have fast-math.`

I'm fine leaving it as-is, but I've found these sorts of things in a handful of places, so if we want to change them, I'll look through the patch more thoroughly, and identify each one I find.


================
Comment at: test/Assembler/fast-math-flags.ll:97
+  %b = fmul reassoc float %x, %y
+; CHECK: %c = call reassoc float @foo(float %b)
+  %c = call reassoc float @foo(float %b)
----------------
Is testing the 'reassoc' flag on the 'call' instruction really what you intended here?  I would have thought add/sub/mul/div, but 'call' surprised me.


https://reviews.llvm.org/D39304