[PATCH] D67351: [InstCombine] Use SimplifyFMulInst to simplify multiply in fma.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 9 13:51:15 PDT 2019
fhahn marked 4 inline comments as done.
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2247-2252
+ Value *RHS, *LHS;
+ if (match(V, m_FMul(m_Value(LHS), m_Value(RHS)))) {
+ II->setArgOperand(0, LHS);
+ II->setArgOperand(1, LHS);
+ return II;
+ }
----------------
spatel wrote:
> I don't understand what this block does. How does an fmul simplify to a different fmul? And then it becomes LHS^2?
> I don't understand what this block does. How does an fmul simplify to a different fmul?
If we simplify to a different fmul, we can update the existing FMA and we do not have to replace it with a new fmul + fadd. Not sure if that happens in practice though. Simplifications like the ones below ( -x * -y => x * y) could result in a different fmul. But currently we do not do this in SimplifyFMulInst. If there is a reason we never do create different fmuls there, then we can drop it.
> And then it becomes LHS^2?
That should have been RHS of course.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2260-2262
Value *Src0 = II->getArgOperand(0);
Value *Src1 = II->getArgOperand(1);
Value *X, *Y;
----------------
spatel wrote:
> It would be easier to read if you move the simplify call below these local variables and then re-use those names.
Sure, I've moved it down.
================
Comment at: llvm/test/Transforms/InstCombine/fma.ll:185
; CHECK-LABEL: @fmuladd_fneg_x_fneg_y_fast(
-; CHECK-NEXT: [[TMP1:%.*]] = fmul fast float [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT: [[TMP1:%.*]] = fmul fast float [[Y:%.*]], [[X:%.*]]
; CHECK-NEXT: [[FMULADD:%.*]] = fadd fast float [[TMP1]], [[Z:%.*]]
----------------
spatel wrote:
> I'm not seeing how these got commuted.
That was noise of update_test_checks.py I think. The way it generates checks for arguments, it will match both `fmul %x, %y` and `fmul %y, %x`.
I've stripped the unnecessary changes.
================
Comment at: llvm/test/Transforms/InstCombine/fma.ll:372-375
+define <2 x double> @fmuladd_a_0_b(<2 x double> %a, <2 x double> %b) {
+; CHECK-LABEL: @fmuladd_a_0_b(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: ret <2 x double> [[B:%.*]]
----------------
spatel wrote:
> Please add all of the new tests with baseline results as a preliminary commit, so we just have the diffs here.
> Add one more test like:
> call fast fma(sqrt(X), sqrt(X), Y) --> X*Y
Done
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67351/new/
https://reviews.llvm.org/D67351
More information about the llvm-commits
mailing list