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

[Arnaud A. de Grandmaison]

On 02/15/2013 01:47 PM, Duncan Sands wrote:
> 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.

I do not think "sr" to be readible, so I rephrased the comment to "trunc
(CI>>N)"

>
>> +    // 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.
>

Done

>> +    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);
>

Took the second option, with the tweak that Amt can not be directly used
: ConstantInt(Amt) is passed instead.

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

Did not apply it yet, in case you have further remarks.

Thanks for your time and the review.

Cheers,
--
Arnaud

> 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: 4366 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130215/29a70b3b/attachment.bin>