[llvm] r287014 - Revert r286999 which caused buildbot test failures. Some testcases need to be made target specific.

[Wei Mi via llvm-commits]

Hi Renato,

What I meant is "test/Transforms/LoopStrengthReduce/nested-loop.ll"
should be target specific, not
test/CodeGen/AArch64/eliminate-trunc.ll. nested-loop.ll failed on some
targets but passed on AArch64.

I feel my patch generates reasonable code for
test/CodeGen/AArch64/eliminate-trunc.ll:

LSR trace without my patch. The trace shows the final decision about
how to generate induction variables for LSR.
The chosen solution requires 2 regs, with addrec cost 2, plus 1 setup cost:
  LSR Use: Kind=Basic, Offsets={0}, widest fixup type: i32
    reg({0,+,1}<nuw><nsw><%for.body4.us>)
  LSR Use: Kind=Address of i8 in addrspace(0), Offsets={0,0}, widest
fixup type: i8*
    reg({{%a,+,8}<nsw><%for.body4.lr.ph.us>,+,1}<nw><%for.body4.us>)

LSR trace with my patch:
The chosen solution requires 2 regs, with addrec cost 1:
  LSR Use: Kind=Basic, Offsets={0}, widest fixup type: i32
    reg({0,+,1}<nuw><nsw><%for.body4.us>)
  LSR Use: Kind=Address of i8 in addrspace(0), Offsets={0,0}, widest
fixup type: i8*
    reg({%a,+,8}<nsw><%for.body4.lr.ph.us>) +
1*reg({0,+,1}<nuw><nsw><%for.body4.us>)

What my patch does is to allow the formula including induction value
from outerloop as a separate Reg which was dropped unconditionally
before -- reg({%a,+,8}<nsw><%for.body4.lr.ph.us>) +
1*reg({0,+,1}<nuw><nsw><%for.body4.us>). It provides more alternative
for LSR to choose, and apparently LSR chooses it in the final
decision, because it will generate one less addrec operation.

However, although there is one less addrec operation in IR after LSR,
there is no much difference in the final assembly w/wo my patch --
same instruction number. That is because AArch64 supports
post-load/store increment, which hides the cost of addrec.

innerloop code without my patch.
LBB0_3:                                 ; %for.body4.us
                                        ;   Parent Loop BB0_2 Depth=1
                                        ; =>  This Inner Loop Header: Depth=2
  ldrb w12, [x11]
  ldrb w12, [x1, x12]
  strb w12, [x11], #1
  add w10, w10, #1            ; =1
  cmp w10, w8
  b.lt LBB0_3

innerloop code with my patch.
LBB0_3:                                 ; %for.body4.us
                                        ;   Parent Loop BB0_2 Depth=1
                                        ; =>  This Inner Loop Header: Depth=2
  ldrb w11, [x0, x10]
  ldrb w11, [x1, x11]
  strb w11, [x0, x10]
  add x10, x10, #1            ; =1
  cmp w10, w8
  b.lt LBB0_3

So after the change, the test fails just because the precondition of
the test disappears. It is not the patch generating unreasonable code.
I try some effort to change the test and prevent my patch from kicking
in, like if I change the test by removing its outerloop (but keep the
innerloop the same), it will still use reg({0,+,1}) in both LSR uses
and the test still fails. It further proves that the test can pass
before only because the limitation about formula with induction value
Reg from outerloop exists.

Do you have any suggestions about how to change the test, or is it ok
to remove it?

Thanks,
Wei.



On Tue, Nov 15, 2016 at 11:59 AM, Renato Golin <renato.golin at linaro.org> wrote:
> On 15 November 2016 at 19:42, Wei Mi via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>> Author: wmi
>> Date: Tue Nov 15 13:42:05 2016
>> New Revision: 287014
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=287014&view=rev
>> Log:
>> Revert r286999 which caused buildbot test failures. Some testcases need to be made target specific.
>
> Hi Wei,
>
> I'm not sure making tests target specific will fix anything. One of
> the failures was that a truncation shouldn't have happened and it did.
>
> I think there's more than meets the eye. On your next round, please
> include Tim, James and me on the review, so we can understand the
> changes to the tests and the code better.
>
> cheers,
> --renato