[PATCH] D26781: [LSR] Canonicalize formula and put recursive Reg related with current loop in ScaledReg.

[Anna Thomas via llvm-commits]

sgtm! thanks
> On May 18, 2017, at 1:06 PM, Wei Mi <wmi at google.com> wrote:
> 
> Hi Anna,
> 
> I just finish some testing and about to checkin the fix now (plan not
> to send out a separate review since it is a very safe change).
> 
> Wei.
> 
> On Thu, May 18, 2017 at 10:02 AM, Anna Thomas <anna at azul.com> wrote:
>> Hi Wei,
>> 
>> Thanks for looking into this. Is this already fixed?
>> 
>> 
>> Anna
>> 
>>> On May 17, 2017, at 5:48 PM, Wei Mi <wmi at google.com> wrote:
>>> 
>>> Quentin, I think you are right.
>>> 
>>> Here is what is happening:
>>> 
>>> This is the formula of a LSR Use, where multiple induction variables
>>> for the same loop appears in the same formula.
>>> 
>>> Initial formula of LSR Use. Look at the first Reg, the SCEVAddExprRec
>>> is inside an SCEVMulExpr, so it is not interesting as for IVUser
>>> analysis. The second SCEVAddExprRec is interesting for IVUser
>>> analysis. That is why the use is considered as a IVUse.
>>> LSR Use: Kind=Basic, Offsets={0}, widest fixup type: i32
>>> reg(((2 * ((-2 * %5) + (-1 * %8) + {(2 + (3 *
>>> undef)),+,8}<%not_zero48.us>)) + (-1 * %11))) + 1*reg({(3 +
>>> undef),+,8}<%not_zero48.us>)
>>> 
>>> In GenerateAllReuseFormulae, it becomes:
>>> reg(((-4 * %5) + (-2 * %8) + (-1 * %11))) + reg((3 + undef)) + reg((6
>>> * undef)) + reg({0,+,8}<%not_zero48.us>) +
>>> 1*reg({4,+,16}<%not_zero48.us>)
>>> Now there are two interesting SCEVAddExprRec from the same loop.
>>> 
>>> In GenerateTruncates, it tries to signextend the ScaledReg of the formula:
>>> reg((zext i32 ((-4 * %5) + (-2 * %8) + (-1 * %11)) to i64)) +
>>> reg((zext i32 (3 + undef) to i64)) + reg((zext i32 (6 * undef) to
>>> i64)) + reg({0,+,8}<nw><%not_zero48.us>) + 1*reg((4 + (sext i32
>>> {0,+,16}<%not_zero48.us> to i64))<nsw>)
>>> 
>>> After sign-extension, the ScaledReg in the formula is not a
>>> SCEVAddRecExpr anymore. However we have another SCEVAddRecExpr type of
>>> reg: reg({0,+,8}<nw><%not_zero48.us>), so canonicalization think maybe
>>> it is better to use reg({0,+,8}<nw><%not_zero48.us>) as the ScaledReg
>>> in the formula.
>>> 
>>> I think it makes sense to add a canonicalization call in
>>> GenerateTruncates for it to be more robust to handle special case like
>>> this. I am not sure how common the case is, as a possible enhancement
>>> for canonicalization, merging multiple SCEVAddExprRec from the same
>>> loop may be considered.
>>> 
>>> With the information above, if it is still ok, I am going to add the
>>> canonicalization call in GenerateTruncates, and a testcase.
>>> 
>>> Thanks,
>>> Wei.
>>> 
>>> On Wed, May 17, 2017 at 11:47 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> I am guessing we are missing a call to canonicalize after we changed a formula.
>>>> 
>>>>> On May 17, 2017, at 11:45 AM, Wei Mi <wmi at google.com> wrote:
>>>>> 
>>>>> Hi Anna,
>>>>> 
>>>>> Sorry to cause the regression. I am looking at the testcase right now
>>>>> you provided in the PR. I will revert the change if I cannot find a
>>>>> quick fix.
>>>>> 
>>>>> Wei.
>>>>> 
>>>>> On Wed, May 17, 2017 at 10:54 AM, Anna Thomas via Phabricator
>>>>> <reviews at reviews.llvm.org> wrote:
>>>>>> anna added a comment.
>>>>>> 
>>>>>> This patch caused an assertion failure as filed in https://bugs.llvm.org/show_bug.cgi?id=33077
>>>>>> @wmi: Could you please revert this change?
>>>>>> 
>>>>>> 
>>>>>> Repository:
>>>>>> rL LLVM
>>>>>> 
>>>>>> https://reviews.llvm.org/D26781
>>>>>> 
>>>>>> 
>>>>>> 
>>>> 
>>