[PATCH] Move transforms from InstCombine to InstSimplify

[David Majnemer]

On Thu, Jul 11, 2013 at 3:18 PM, David Majnemer <david.majnemer at gmail.com>wrote:

> On Thu, Jul 11, 2013 at 12:49 AM, Duncan Sands <duncan.sands at gmail.com>wrote:
>
>> Hi David,
>>
>>
>> On 11/07/13 00:26, David Majnemer wrote:
>>
>>> InstCombine was pattern matching for (icmp (shl 1, X), Cst) to try to
>>> transform
>>> these icmps to true/false if we could prove that Cst must be in/out of
>>> range.
>>>
>>> However, there are two problems:
>>> 1. This sort of thing belongs in InstSimplify
>>> 2. This can be generalized further
>>>
>>> Attached is a patch that moves over these transforms to
>>> InstructionSimplify and
>>> handles arbitrary constants on the left hand side of the shift and
>>> constrains
>>> the possible range further by analyzing the nsw/nuw flags on the shift.
>>>
>>
>>  --- lib/Analysis/**InstructionSimplify.cpp        (revision 186043)
>>> +++ lib/Analysis/**InstructionSimplify.cpp        (working copy)
>>> @@ -1995,6 +1995,27 @@
>>>          Lower = IntMin.sdiv(Val);
>>>          Upper = IntMax.sdiv(Val) + 1;
>>>        }
>>> +    } else if (match(LHS, m_Shl(m_ConstantInt(CI2), m_Value()))) {
>>> +      const APInt &CI2Value = CI2->getValue();
>>> +      if (cast<**OverflowingBinaryOperator>(**LHS)->hasNoUnsignedWrap())
>>> {
>>> +        // 'shl nuw CI2, x' produces [CI2, CI2 << CLZ(CI2)]
>>> +        Lower = CI2Value;
>>> +        Upper = (CI2Value << CI2Value.countLeadingZeros()) + 1;
>>> +      } else if (cast<**OverflowingBinaryOperator>(**LHS)->hasNoSignedWrap())
>>> {
>>> +        if (CI2->isNegative()) {
>>> +          // 'shl nsw CI2, x' [CI2 << (CLO(CI2)-1), CI2]
>>> +          Lower = (CI2Value << (CI2Value.countLeadingOnes() - 1));
>>> +          Upper = CI2Value + 1;
>>> +        } else {
>>> +          // 'shl nsw CI2, x' [CI2, CI2 << (CLZ(CI2)-1)]
>>> +          Lower = CI2Value;
>>> +          Upper = (CI2Value << (CI2Value.countLeadingZeros() - 1)) + 1;
>>> +        }
>>>
>>
>> the cases above look OK, but
>>
>>  +      } else {
>>> +        // 'shl CI2, x' produces [CI2, CI2 << (Width-1)]
>>> +        Lower = CI2Value;
>>> +        Upper = CI2Value.shl(Width - 1) + 1;
>>>
>>
>> this one seems wrong to me.  Consider for example CI2 (in binary) equal to
>>   10001000
>> Then Lower=10001000, Upper=00000001, but a left-shift by 1 of CI2 is
>> 00010000 which is not in the range Lower .. Upper.
>>
>
> Thanks, I've updated the patch so that we will set Lower to zero and Upper
> to CI2 << CLZ(CI2) which is conservative but, AFAIK, correct.
>

Sorry for the noise but I was able to tighten the bounds if CI2 is a power
of two.


>
>
>>
>> Ciao, Duncan.
>> ______________________________**_________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/**mailman/listinfo/llvm-commits<http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130711/863dd3e8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shfcmpv3.diff
Type: application/octet-stream
Size: 5362 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130711/863dd3e8/attachment.obj>