[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
Tue Oct 31 19:08:57 PDT 2017
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
More information about the llvm-commits
mailing list