[PATCH] D67351: [InstCombine] Use SimplifyFMulInst to simplify multiply in fma.

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 9 15:11:02 PDT 2019


spatel 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;
+      }
----------------
fhahn wrote:
> 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. 
I think we should drop this clause...unless there is a test that shows the intended transform? 
InstSimplify never creates new instructions because it can be used as an analysis rather than transform pass. 
If we fold to a different multiply, then it should be optimized just as well as. 
Eg, we have this fold in InstCombiner::visitFMul():
  // -X * -Y --> X * Y
(can't do that in instsimplify because it's a new instruction)


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