[PATCH] D151616: [Transforms][Reassociate] "Reassociate expressions" pass optimizations not always profitable

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1705
+      BinaryOperator *BOp =
+          isReassociableOp(Ops[i].Op, Instruction::Mul, Instruction::FMul);
+      if (!BOp)
----------------
Although it is less performance sensitive, pulling adds in a loop could also be problematic.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1709
+
+      if (Loop *L = LI.getLoopFor(BOp->getParent())) {
+        // Make sure we're not pulling operands from the outside of the loop.
----------------
Early exit to reduce indentation


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1719
+            if ((LI.getLoopFor(I->getParent()) != L) || (dyn_cast<PHINode>(I)))
+              return false;
+            for (Value *Operand : I->operands()) {
----------------
Shouldn't PHIs with the same loop as L return `true` here?


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1742
+          if ((hasFactor(Operand, MaxOccVal)) && (!isInOneLoop(LI, Operand, L)))
+            return nullptr; // Transformation not profitable, giving up.
+        }
----------------
TL;DR I think the model should be more general than that but as it is, it is probably a good proxy for not shooting ourselves in the foot.

The problem is not pulling something into a loop, the problem is replacing X something with 1 * frequency-of-the-target-basic-block something.

I.e., let's imagine we have:
v1 = s1  * delta
v2 = s2 * delta
...
vN = sN *delta
for () {
  v1 + ... + vN // with loop dependent stuff
}

Transforming this into:
for () {
  (s1 + ... + sN) * delta
}

Could actually be beneficial as long as the loop count is smaller than N.

Similarly, something like:
for () {
  if (unlikely)
   v1 + ... + vN
}

Is even more likely to be beneficial since the expected frequency of the basic block may be smaller than the source basic block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151616