<div dir="ltr">Hi Yi,<div><br></div><div><div>Both LAdd and RAdd come from a dyn_cast but are immediately sent to a call to match. Are you asserting that the dyn_cast could never fail? If so, please replace the dyn_casts with cast.<br>
</div></div><div><br></div><div>The definition of MaxAddCst can be sinked inside of the 'if (LAddOpnd == RAddOpnd)' check.</div><div><br></div><div>DiffCst is required to be a power of 2 for this transform to work, I think it would make sense to perform that check separately and before a lot of the other APInt math; APInts may hold numbers larger than 2**64, it would be nice if we could avoid the big-int slow path code where we can. In fact, it would be nice to compute as little as possible as we go along making checks.</div>
<div><br></div><div>The line computing RangeDiff is considerably more than 80 columns, can you please run clang-format on that line?</div><div><br></div><div>Finally, please remove the trailing whitespace from your combine.</div>
<div><br></div><div>Please keep in mind our developer policy when pinging patches: <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a></div><div><br>
</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 13, 2014 at 12:59 PM, Yi Jiang <span dir="ltr"><<a href="mailto:yjiang@apple.com" target="_blank">yjiang@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ping ...<br>
<div class="HOEnZb"><div class="h5">On Aug 11, 2014, at 10:19 AM, Yi Jiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:<br>
<br>
> Hi Nuno,<br>
> Yes. it does. We have already extracted the test case to reflect the pattern we want to catch most and will go with the patch. We believe the most useful case is to optimize:<br>
> ((c) >= 'A' && (c) <= 'Z') || ((c) >= 'a' && (c) <= 'z’). Thank you.<br>
><br>
> -Yi<br>
><br>
> On Aug 11, 2014, at 9:17 AM, Nuno Lopes <<a href="mailto:nuno.lopes@ist.utl.pt">nuno.lopes@ist.utl.pt</a>> wrote:<br>
><br>
>> AFAICT, correctness wise the patch looks good. I've translated your optimization into Alive by hand and now it seems good.<br>
>> Now I wonder whether the optimization still kicks in on your benchmarks? (given that now the constraints are much thigher)<br>
>><br>
>> Nuno<br>
>><br>
>> ----- Original Message ----- From: "Yi Jiang" <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>><br>
>> Sent: Thursday, August 07, 2014 8:37 AM<br>
>> Subject: Re: [Patch]New InstCombine pattern for Icmp<br>
>><br>
>><br>
>>> With the help of Nuno and the Alive tool, I figured out the new condition. The original condition actually refers to the following two ranges:<br>
>>> (icmp ult/ule (A + C1), C3) | (icmp ult/ule (A + C2), C3) == [MAX_UINT-C1+1, MAX_UINT-C1+1+C3] and [MAX_UINT-C2+1, MAX_UINT-C2+1+C3]<br>
>>> We can fold these two range sif:<br>
>>> 1) C1 and C2 is unsigned greater than C3<br>
>>> 2) The two ranges are separated. abs(LowRange1-LowRange2) > C3<br>
>>> 3) C1 ^ C2 is one-bit mask.<br>
>>> 4) LowRange1 ^ LowRange2 and HighRange1 ^ HighRange2 are one-bit mask.<br>
>>><br>
>>> Here is the new patch:<br>
>>><br>
>>><br>
>>> On Jul 30, 2014, at 2:24 PM, Nuno Lopes <<a href="mailto:nuno.lopes@ist.utl.pt">nuno.lopes@ist.utl.pt</a>> wrote:<br>
>>><br>
>>>> Hi Yi,<br>
>>>><br>
>>>> ALIVe still complains, I'm afraid:<br>
>>>><br>
>>>> Precondition: isPowerOf2(C1 ^ C2) && ugt(C1, C3) && ugt(C2, C3)<br>
>>>> %x = add %A, C1<br>
>>>> %i = icmp ult %x, C3<br>
>>>> %y = add %A, C2<br>
>>>> %j = icmp ult %y, C3<br>
>>>> %r = or %i, %j<br>
>>>> =><br>
>>>> %and = and %A, ~(C1 ^ C2)<br>
>>>> %lhs = add %and, umax(C1, C2)<br>
>>>> %r = icmp ult %lhs, C3<br>
>>>><br>
>>>> Done: 1<br>
>>>> ERROR: Mismatch in values of i1 %r<br>
>>>><br>
>>>> Example:<br>
>>>> %A i2 = 2 (0x2)<br>
>>>> C1 i2 = 3 (0x3)<br>
>>>> %x i2 = 1 (0x1)<br>
>>>> C3 i2 = 1 (0x1)<br>
>>>> %i i1 = 0 (0x0)<br>
>>>> C2 i2 = 2 (0x2)<br>
>>>> %y i2 = 0 (0x0)<br>
>>>> %j i1 = 1 (0x1)<br>
>>>> %and i2 = 2 (0x2)<br>
>>>> %lhs i2 = 1 (0x1)<br>
>>>> Source value: 1 (0x1)<br>
>>>> Target value: 0 (0x0)<br>
>>>><br>
>>>><br>
>>>> Nuno<br>
>>>><br>
>>>> ----- Original Message ----- From: "Yi Jiang" <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>><br>
>>>> To: "Nuno Lopes" <<a href="mailto:nuno.lopes@ist.utl.pt">nuno.lopes@ist.utl.pt</a>><br>
>>>> Cc: "LLVM Commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
>>>> Sent: Wednesday, July 30, 2014 8:28 PM<br>
>>>> Subject: Re: [Patch]New InstCombine pattern for Icmp<br>
>>>><br>
>>>><br>
>>>> Hi Nuno,<br>
>>>><br>
>>>> Thank you for pointing out this! One condition is missing: C1 and C2 should be unsigned greater than C3.<br>
>>>> Here is the new patch.<br>
>>>><br>
>>>> -Yi<br>
>>>><br>
>>>> On Jul 29, 2014, at 11:34 AM, Nuno Lopes <<a href="mailto:nuno.lopes@ist.utl.pt">nuno.lopes@ist.utl.pt</a>> wrote:<br>
>>>><br>
>>>>> Hi,<br>
>>>>><br>
>>>>> I sent you an email the other day, but it was rejected by Apple's email server. Tyring now through another email account.<br>
>>>>> Please take a look here: <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140721/227575.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140721/227575.html</a><br>
>>>>><br>
>>>>> Nuno<br>
>>>>><br>
>>>>> ----- Original Message -----<br>
>>>>><br>
>>>>> Ping...<br>
>>>>> On Jul 25, 2014, at 4:11 PM, Yi Jiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:<br>
>>>>><br>
>>>>>> Hi Ben,<br>
>>>>>><br>
>>>>>> Thank you for your comments! Here is a new version. In this version, I still kept some "if" thinking that some operand has been already extracted so I would like to use it directly. Please let me know if any other comments.<br>
>>>>>> <combinecmpV2.patch><br>
>>>>>> -Yi<br>
>>>>>> On Jul 25, 2014, at 2:12 AM, Benjamin Kramer <<a href="mailto:benny.kra@gmail.com">benny.kra@gmail.com</a>> wrote:<br>
>>>>>><br>
>>>>>>> On Fri, Jul 25, 2014 at 1:24 AM, Yi Jiang <<a href="mailto:yjiang@apple.com">yjiang@apple.com</a>> wrote:<br>
>>>>>>>> Hi,<br>
>>>>>>>><br>
>>>>>>>> his patch is trying to fold (icmp ult/ule (A + C1), C3) | (icmp ult/ule (A +<br>
>>>>>>>> C2), C3) to (icmp ult/ule ((A & ~(C1 ^ C2)) + max(C1, C2)), C3) .<br>
>>>>>>>> This transformation is legal if C1 ^ C2 is one-bit mask, in other word, C1<br>
>>>>>>>> is only one bit different from C2. In this case, we can "mask" that bit and<br>
>>>>>>>> do just one comparison.<br>
>>>>>>>> A typical example is:<br>
>>>>>>>> isALPHA(c) ((c) >= 'A' && (c) <= 'Z') || ((c) >= 'a' && (c) <= 'z')<br>
>>>>>>>><br>
>>>>>>>> Now llvm will optimize it to ((c + 191) <=25) || ((c + 159) <=25)<br>
>>>>>>>><br>
>>>>>>>> With this patch, we can optimize it further to:<br>
>>>>>>>> (c & 223) + 191 <= 25<br>
>>>>>>>><br>
>>>>>>>> The binary format of the constants are:<br>
>>>>>>>> 191 10111111<br>
>>>>>>>> 159 10011111<br>
>>>>>>>> 223 11011111<br>
>>>>>>>><br>
>>>>>>>> Here is some experiment result on arm64:<br>
>>>>>>>> The patch shows no regression and improve spec2000 perlbmk 3.8% in<br>
>>>>>>>> test-suite under -O3.<br>
>>>>>>>> We also test the spec2006 400.perlbench with ref size input, it will improve<br>
>>>>>>>> 1% under -O3 and 1.2% under -O3+lto+pgo.<br>
>>>>>>><br>
>>>>>>> Wow, very nice.<br>
>>>>>>><br>
>>>>>>>> Any comments are appreciated.<br>
>>>>>>><br>
>>>>>>> The patch could be significantly simplified with the PatternMatch<br>
>>>>>>> tool. Something like match(LHS, m_Add(m_OneUse(m_Value(A),<br>
>>>>>>> m_ConstantInt(LAddCst)... could replace your if chain.<br>
>>>>>>><br>
>>>>>>> - Ben<br>
>><br>
><br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>