[PATCH] D149132: [LICM] Reassociate & hoist add expressions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2576
+
+  // LHS itsels if a invariant, try to represent it in the form:
+  // "VariantOp + InvariantOp". If it it possible, then we can reassociate.
----------------
anna wrote:
> Nit: LHS itself is an invariant. 
I must have been crazy when I wrote this. It's not just a bunch of typos, but it's also "variant" instead of "invariant" :)


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2588
+  // flag on operation, and the no-overflow of the new RHS is checked below.
+  if (!cast<OverflowingBinaryOperator>(LHS)->hasNoSignedWrap())
+    return false;
----------------
mkazantsev wrote:
> skatkov wrote:
> > Will SCEV more powerful to check that LHS is NSW then just check of flag on instruction?
> In theory, yes.
Added one more overflow check, leaving nsw as shortcut.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2591-2594
+  const SCEV *InvariantOpS = SE.getNegativeSCEV(SE.getSCEV(InvariantOp));
+  if (!SE.willNotOverflow(Instruction::BinaryOps::Add, /*Signed*/ true, RHSS,
+                          InvariantOpS, /*CtxI*/ &I))
+    return false;
----------------
mkazantsev wrote:
> nikic wrote:
> > This seems more obvious and matches the generated sub?
> Yes, and it provides worse results on the tests that are here.
No longer actual after API change.


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

https://reviews.llvm.org/D149132