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

[Wei Mi via llvm-commits]

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