[PATCH] D152281: [Transforms][LICM] Add the ability to undo unprofitable reassociation

[Paul Osmialowski via Phabricator via llvm-commits]

pawosm01 marked 2 inline comments as done.
pawosm01 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2708
+      return false;
+    if (!match(VOp, m_FAdd(m_BinOp(Op), m_BinOp(OpNext)))) {
+      Op = VOp;
----------------
nikic wrote:
> Do we need to require that the fadd is reassoc as well?
Yes, this match specifically requires that the expression is of the form that is described in the comment written above this function: `((A1 * B1) + (A2 * B2) + ...)`. I wrote this function to solve the problem at hand, to which I know that my solution provides noticeable performance benefit. Trying to make it more general would require a whole lot more research and performance impact analysis that would go far beyond what I wanted to fix. I'm offering this solution which I based on the other hoist... functions in this file, I believe if one finds a reason for extending this (to handle their own performance issue at hand), they could do that basing on what I did propose.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2742
+    IRBuilder<> Builder(Preheader->getTerminator());
+    U->set(Builder.CreateFMulFMF(U->get(), Factor, &I, "factor.op.fmul"));
+  }
----------------
nikic wrote:
> This copies the FMF flags from the top-level fmul, but is that correct? We have multiple fadds and fmuls involved in the transform, why is it safe to use the flags from that one in particular?
> 
> (The test coverage for this is not great, because you just use "fast" everywhere.)
Yes I assume it's correct. What is happening here is that the fmul instruction from which the flag is taken is effectively being copied to every subexpression in the parenthesis, it is reasonable to copy the fast flag too. As checking for associations being allowed is implicitly checking the fast flag, nothing harmful is happening here.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152281/new/

https://reviews.llvm.org/D152281