[PATCH] D124369: [InstCombine] C0 >>{ashr, exact} (X - C1) --> (C0 << C1) >>{ashr} X

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I think this is correct and sufficiently tested now. See inline comments for a couple of nits. Once those are addressed, I can push the patch on your behalf.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:426-427
+
+    auto isSuitableForPreShift = [PosOffset, &I,
+                                  AC](Instruction::BinaryOps BinOpcode) {
+      switch (BinOpcode) {
----------------
This is more verbose than needed - if we're using &I, then we can just grab the opcode from it via I.getOpcode().
So we can either pass things in as params or use refs, instead of partly using both ways?
    auto isSuitableForPreShift = [](Instruction &I, const APInt *AC, unsigned PosOffset) {
    auto isSuitableForPreShift = [&I, AC, PosOffset]() {



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:458
+          BinaryOperator::Create(I.getOpcode(), NewC, A);
+      applyFlag(NewShiftOp);
+      return NewShiftOp;
----------------
Instead of a lambda + switch for setting the flags, I'd prefer the shorter and more direct:
     if (I.getOpcode() == Instruction::Shl) {
        NewShiftOp->setHasNoUnsignedWrap(I.hasNoUnsignedWrap());
     else
        NewShiftOp->setIsExact();


That might tilt the whole thing towards a lambda-less implementation, but you can decide which is easier to read.


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

https://reviews.llvm.org/D124369