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

[Wei Mi via llvm-commits]

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
>>>>>
>>>>>
>>>>>
>>>
>