[PATCH] D63812: [InstCombine] Shift amount reassociation (PR42391)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Thanks for the review!

Other than D63829 <https://reviews.llvm.org/D63829>, unless that is magically sufficient to fix all the
problems i'm observing, i suspect i will have some more folds like this...



================
Comment at: lib/Transforms/InstCombine/InstCombineShifts.cpp:37
+    return nullptr;
+  auto *Sh1 = cast<BinaryOperator>(Sh1Value);
+
----------------
nikic wrote:
> Personally I'd prefer `auto *Sh1 = cast<BinaryOperator>(Sh0->getOperand(0))` over the use of `m_CombineAnd()` here, which makes the match less straightforward.
It really differs on case-by-case basis. I think `m_CombineAnd()` may be better in general,
at least compared to backend lack of it.


================
Comment at: lib/Transforms/InstCombine/InstCombineShifts.cpp:41
+  Instruction::BinaryOps ShiftOpcode = Sh0->getOpcode();
+  if (ShiftOpcode != cast<Instruction>(Sh0->getOperand(0))->getOpcode())
+    return nullptr;
----------------
nikic wrote:
> Can replace `cast<Instruction>(Sh0->getOperand(0))` with `Sh1` here.
Whoops, yes, i wanted to do that but forgot. Thank you.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63812