[Patch] Teach InstCombine to work with smaller legal types in icmp (shl %v, C1), C2

[Duncan Sands]

Hi Arnaud,

> --- a/lib/Transforms/InstCombine/InstCombineCompares.cpp
> +++ b/lib/Transforms/InstCombine/InstCombineCompares.cpp
> @@ -1331,6 +1331,22 @@ Instruction *InstCombiner::visitICmpInstWithInstAndIntCst(ICmpInst &ICI,
>        return new ICmpInst(TrueIfSigned ? ICmpInst::ICMP_NE : ICmpInst::ICMP_EQ,
>                            And, Constant::getNullValue(And->getType()));
>      }
> +
> +    // Transform (icmp pred iM (shl iM %v, N), CI)
> +    // -> (icmp pred i(M-N) (trunc %v iM to i(N-N)), (asr CI, N))
> +    // Transform the shl to a trunc if (asr CI, N) has no loss.

asr -> asr/lsr (or maybe just "sr") as the fact it is an arithmetic shift isn't
relevant and may be confusing.

> +    // This enables to get rid of the shift in favor of a trunc which can be
> +    // free on the target. It has the additional benefit of comparing to a
> +    // smaller constant, which will be target friendly.
> +    unsigned Amt = ShAmt->getLimitedValue(TypeBits);

Please use TypeBits-1 here.  That way anyone reading this can see, using this
local information, that no nasty crashes can occur.  As you pointed out, they
can't happen because of the conditions checked earlier, i.e. because of more
global information.  As being able to tell locally that invariants hold is
generally superior, and it's easy to do, I'd prefer it with TypeBits-1.

> +    if (Amt != 0 && RHSV.countTrailingZeros() >= Amt) {
> +      Type *NTy = IntegerType::get(ICI.getContext(), TypeBits - Amt);
> +      Constant *NCI = ConstantExpr::getAShr(RHS, ShAmt);

ShAmt -> Amt.  Also, please add a little comment that the A in AShr isn't
relevant, for example:
   Constant *NCI = ConstantExpr::getAShr(RHS, Amt); // LShr would also be OK.
Alternatively, do the truncation here too so it is more obvious that the shift
type doesn't matter, for example:
   Constant *NCI = ConstantExpr::getTrunc(ConstantExpr::getAShr(RHS, Amt), NTy);

> +      return new ICmpInst(ICI.getPredicate(),
> +                          Builder->CreateTrunc(LHSI->getOperand(0), NTy),
> +                          ConstantExpr::getTrunc(NCI, NTy));
> +    }
> +
>      break;
>    }

OK to apply with these changes.

Ciao, Duncan.