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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: include/llvm/IR/Operator.h:187
+    ApproxFunc      = (1 << 6)
   };
 
----------------
wristow wrote:
> 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.
I want to fix the backend too, but that's separate patches. For example, we don't propagate the IR flags properly yet (see D37686). 

Also IIRC, the backend does not treat the global "-enable-unsafe-fp-math" as an umbrella for other flags. So  that setting does not imply "-enable-no-nans-fp-math" or other FP relaxations.


================
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())
----------------
wristow wrote:
> 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.
I'll fix the comment to match the current code. Since this is the reassociation pass, I would guess that 'reassoc' is all we need to enable transforms here, but we'll have to verify that that is correct.


================
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)
----------------
wristow wrote:
> 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.
It is surprising but intentional. I want to highlight the fact that we allow any FMF on any FPMathOperator. So things like this or 'fadd arcp ...' are legal but I'm not sure how that would be used for optimization. We may want to refine that someday?


https://reviews.llvm.org/D39304