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

[Arnaud A. de Grandmaison]

Patch updated according to your comments. See my answers below.

On 02/14/2013 09:56 PM, Duncan Sands wrote:
> Hi Arnaud,
>
>> @@ -1331,6 +1331,24 @@ 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.
>> +    // 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.
>> +    if (ShAmt->getZExtValue() &&
>
> you can use !ShAmt->isZero().  I don't like getZExtValue() because it
> will crash
> if the value doesn't fit in 64 bits, which is theoretically possible. 
> You can
> also use !RHS->isNullValue().  

Ok.

> Also, won't ShAmt->getZExtValue() crash if this
> is a vector shift?
>

ShAmt is enforced to be a ConstantInt a few lines upper, when entering
the  "(icmp pred (shl X, ShAmt), CI)" case.
I am not familiar with how vector are handled; looking at the function,
nobody seems to care --- which is not an excuse :), but should be a
different patch.

It is also checked when entering the case that the ShAmt is in range,
i.e. no undefined shift can occur.

> Alternatively, you could do:
>   unsigned BitWidth = RHS->getType()->getPrimitiveSizeInBits();
>   unsigned Amt = ShAmt->getLimitedValue(BitWidth);
> Then use Amt rather than ShAmt->getZExtValue() everywhere.  This is
> valid since
> if the shift amount is bigger than the bitwidth then the shift represents
> undefined behaviour.  Maybe better is to use BitWidth-1, since that is
> still OK
> and avoids the possible DestTypeSize being zero problem described below.

ShAmt < TypeBits, so this can not happen.

>
>> +        RHS->getValue().countTrailingZeros() >=
>> ShAmt->getZExtValue()) {
>
>
>> +      unsigned DestTypeSize =
>> +        LHSI->getType()->getPrimitiveSizeInBits() -
>> ShAmt->getZExtValue();
>
> If RHS is zero (so countTrailingZeros is equal to the bitwidth), and
> ShAmt->getZExtValue() is equal to the bitwidth, then DestTypeSize will
> be zero and you will crash below.  

Can not happen (see above)

> Also, since everything so far was in
> terms of RHS, it seems odd to pass to LHSI, you could use RHS->getType().

Ok. This has been reworded.

Cheers,
--
Arnaud
>> +      Type *NTy = IntegerType::get(LHSI->getContext(), DestTypeSize);
>> +      Constant *NCI = ConstantExpr::getAShr(RHS, ShAmt);
>> +      return new ICmpInst(ICI.getPredicate(),
>> +                          Builder->CreateTrunc(LHSI->getOperand(0),
>> NTy),
>> +                          ConstantExpr::getTrunc(NCI, NTy));
>> +    }
>> +
>>      break;
>>    }
>>
>
> Ciao, Duncan.


-- 
Arnaud A. de Grandmaison

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Teach-InstCombine-to-work-with-smaller-legal-types-i.patch
Type: text/x-patch
Size: 4217 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130215/b1c2d03a/attachment.bin>