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

Warren Ristow via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 2 10:55:17 PDT 2017


wristow added inline comments.


================
Comment at: include/llvm/IR/Operator.h:187
+    ApproxFunc      = (1 << 6)
   };
 
----------------
spatel wrote:
> 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.
I see.  Thanks for clarifying.


================
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())
----------------
spatel wrote:
> 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.
To be clear, I'm //not //suggesting that in this patch we change the code here to check for 'reassoc' (i.e., I'm not suggesting we change the `isFast()` call to `hasAllowReassoc()` at this time).  I view that as a separate piece of work, where we go through and carefully audit existing FMF-related checks, and decide how to use the more precise flags.  Possibly it's just 'reassoc' that is needed for this case, or possibly it's 'reassoc' and some other conditions.

All I was suggesting by my comment/question, is that code-comments referring to a no longer-existing "unsafe algebra" umbrella flag, are a bit misleading.  So I wondered whether we wanted to change those comments to better match the new implementation.  It's a pretty minor point, in my view.

>From my POV, the main purpose of this patch is to fix the underlying implementation to allow us to go through and do that audit, and fix issues like this "use `isFast()` or use some finer check?" example, here.


================
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)
----------------
spatel wrote:
> 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?
OK, thanks for explaining.


https://reviews.llvm.org/D39304





More information about the llvm-commits mailing list