[PATCH] InstCombine: Improve the result bitvect type when folding (cmp pred (load (gep GV, i)) C) to a bit test.

[Duncan Sands]

Hi Ahmed,

> This patch makes InstCombine choose better types when folding (cmp
> pred (load (gep GV, i)) C) to a bit test (((bitvector >> i) & 1) !=
> 0).
>
> As the commit says:
>
>> The original code used i32, and i64 if legal. This introduced unneeded
>> casts when they aren't legal, or when the index variable i has another
>> type. In order of preference: try to use i's type, looking past a cast
>> if there is one;  use the smallest fitting legal type (using an added
>> DataLayout method); default to i32.
>> A testcase checks that this works when everything is i16.
>
> I'm not convinced about adding DataLayout::getSmallestLegalIntType,
> but it seemed like the natural way to do it (mirrorring
> isLegalInteger).


> --- a/include/llvm/IR/DataLayout.h
> +++ b/include/llvm/IR/DataLayout.h
> @@ -350,6 +350,9 @@ public:
>    /// type.
>    Type *getIntPtrType(Type *) const;
>
> +  /// getSmallestLegalIntType - Return an integer type with size

This comment is incomplete.

> +  Type *getSmallestLegalIntType(LLVMContext &C, unsigned Width = 0) const;
> +
>    /// getIndexedOffset - return the offset from the beginning of the type for
>    /// the specified indices.  This is used to implement getelementptr.
>    uint64_t getIndexedOffset(Type *Ty, ArrayRef<Value *> Indices) const;

> --- a/lib/Transforms/InstCombine/InstCombineCompares.cpp
> +++ b/lib/Transforms/InstCombine/InstCombineCompares.cpp
> @@ -443,20 +443,35 @@ FoldCmpLoadFromIndexedGlobal(GetElementPtrInst *GEP, GlobalVariable *GV,
>    }
>
>
> -  // If a 32-bit or 64-bit magic bitvector captures the entire comparison state
> +  // If a magic bitvector captures the entire comparison state
>    // of this load, replace it with computation that does:
>    //   ((magic_cst >> i) & 1) != 0
> -  if (ArrayElementCount <= 32 ||
> -      (TD && ArrayElementCount <= 64 && TD->isLegalInteger(64))) {
> -    Type *Ty;
> -    if (ArrayElementCount <= 32)
> +  {
> +    Type *Ty = 0;
> +
> +    // Default to either a legal type, or i32
> +    if (TD)
> +      Ty = TD->getSmallestLegalIntType(Init->getContext(), ArrayElementCount);
> +    else if (ArrayElementCount <= 32)
>        Ty = Type::getInt32Ty(Init->getContext());
> -    else
> -      Ty = Type::getInt64Ty(Init->getContext());
> -    Value *V = Builder->CreateIntCast(Idx, Ty, false);
> -    V = Builder->CreateLShr(ConstantInt::get(Ty, MagicBitvector), V);
> -    V = Builder->CreateAnd(ConstantInt::get(Ty, 1), V);
> -    return new ICmpInst(ICmpInst::ICMP_NE, V, ConstantInt::get(Ty, 0));
> +
> +    // Use Idx' type; if it's a cast, look past it

Given that you might have already determined a value for Ty, shouldn't this
logic only apply when Ty is null?  If not, then shouldn't you first check this
case and only do the above Ty logic afterwards (if Ty is null) in order to
reduce unnecessary work.  Also I think you mean "Idx" in the comment not
"Idx'".  Finally, why look through casts of Idx here, why not just let the
optimizers zap the extra cast if they think it is a good idea?

> +    CastInst *IdxC = dyn_cast<CastInst>(Idx);
> +    if (IdxC && IdxC->isIntegerCast() &&
> +        ArrayElementCount <= IdxC->getSrcTy()->getIntegerBitWidth() &&
> +        ShouldOptimizeCast(IdxC->getOpcode(), IdxC->getOperand(0),
> +                           Idx->getType())) {
> +      Ty = IdxC->getSrcTy();
> +      Idx = IdxC->getOperand(0);
> +    } else if (ArrayElementCount <= Idx->getType()->getIntegerBitWidth())
> +      Ty = Idx->getType();

Style: the above line should be in curly brackets {} because the preceding
"if" branch was.

> +
> +    if (Ty != 0) {
> +      Value *V = Builder->CreateIntCast(Idx, Ty, false);
> +      V = Builder->CreateLShr(ConstantInt::get(Ty, MagicBitvector), V);
> +      V = Builder->CreateAnd(ConstantInt::get(Ty, 1), V);
> +      return new ICmpInst(ICmpInst::ICMP_NE, V, ConstantInt::get(Ty, 0));
> +    }
>    }
>
>    return 0;

Lastly, I don't think your testcase tests all cases, for example it doesn't
look like it tests the case when Idx is a cast, so can you please beef it up.

Best wishes, Duncan.