[PATCH] D84664: [InstCombine] Fold shift-select if all the operands are constant

[Matteo Favaro via Phabricator via llvm-commits]

fvrmatteo added a comment.

OK I think I figured it out.

The **hasOneUse** in **canEvaluateShifted** is absolutely necessary because after its evaluation by **FoldShiftByConstant** the original **select** instruction it's replaced with a new shifted **select** instruction, which means that the original users of `%v0 = select i1 %arg, i64 64, i64 0` will now use its shifted version `%v0 = select i1 %arg, i64 1, i64 0` generated by **FoldShiftByConstant**.

The relevant code is the following:

  // See if we can propagate this shift into the input, this covers the trivial
  // cast of lshr(shl(x,c1),c2) as well as other more complex cases.
  if (I.getOpcode() != Instruction::AShr &&
      canEvaluateShifted(Op0, Op1C->getZExtValue(), isLeftShift, *this, &I)) {
    LLVM_DEBUG(
        dbgs() << "ICE: GetShiftedValue propagating shift through expression"
                  " to eliminate shift:\n  IN: "
               << *Op0 << "\n  SH: " << I << "\n");
  
    return replaceInstUsesWith(
        I, getShiftedValue(Op0, Op1C->getZExtValue(), isLeftShift, *this, DL));  // NOTE: here we are replacing the instruction with more than 1 use
  }

>From the debug output we can see that disabling `if (!I->hasOneUse()) return false;` leads to the following (wrong) sequences:

Substitution of `%v0 = select i1 %arg, i64 64, i64 0` with `%v0 = select i1 %arg, i64 1, i64 0`:

  IC: Visiting:   %v1 = lshr exact i64 %v0, 6
  ICE: GetShiftedValue propagating shift through expression to eliminate shift:
    IN:   %v0 = select i1 %arg, i64 64, i64 0
    SH:   %v1 = lshr exact i64 %v0, 6
  IC: ADD:   %v0 = select i1 %arg, i64 64, i64 0
  IC: Replacing   %v1 = lshr exact i64 %v0, 6
      with   %v0 = select i1 %arg, i64 1, i64 0
  IC: Mod =   %v1 = lshr exact i64 %v0, 6
      New =   %v1 = lshr exact i64 %v0, 6
  IC: ERASE   %v1 = lshr exact i64 %v0, 6
  IC: ADD DEFERRED:   %v0 = select i1 %arg, i64 1, i64 0

Transformation of `%v0 = select i1 %arg, i64 1, i64 0` in `%v0 = zext i1 %arg to i64`:

  IC: Visiting:   %v0 = select i1 %arg, i64 1, i64 0
  IC: Old =   %v0 = select i1 %arg, i64 1, i64 0
      New =   <badref> = zext i1 %arg to i64
  IC: ADD:   %v0 = zext i1 %arg to i64
  IC: ERASE   %0 = select i1 %arg, i64 1, i64 0
  IC: Visiting:   %v0 = zext i1 %arg to i64

Wrong input to `%v3 = shl nuw i64 %v0, 57` that will now use `%v0 = zext i1 %arg to i64` instead of the original `%v0 = select i1 %arg, i64 64, i64 0`:

  IC: Visiting:   %v2 = xor i64 %v0, 1
  IC: Visiting:   %v3 = shl nuw i64 %v0, 57
  IC: Mod =   %v3 = shl nuw i64 %v0, 57
      New =   %v3 = shl nuw nsw i64 %v0, 57
  IC: ADD:   %v3 = shl nuw nsw i64 %v0, 57
  IC: Visiting:   %v3 = shl nuw nsw i64 %v0, 57
  IC: Old =   %v3 = shl nuw nsw i64 %v0, 57
      New =   <badref> = select i1 %arg, i64 144115188075855872, i64 0
  IC: ADD:   %v3 = select i1 %arg, i64 144115188075855872, i64 0

@lebedev.ri I think that calling the patched version of ** SimplifySelectsFeedingBinaryOp** (issuing a direct call to **commonShiftTransforms** from **visitLShr**, **visitShl** and **visitAShr** as you suggested) may be the safest solution, as it is meant to precisely simplify this case. Additionally **commonShiftTransforms** will replace the **lshr** instruction (which is the correct one to be simplified) instead of the **select** (used as input).

I also think that we may try to modify the **canEvaluateShifted** and **FoldShiftByConstant** to handle the multiple-uses case, but that seems way more complex as the code around those functions it's really one-use centric.


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

https://reviews.llvm.org/D84664