handleMoveUp fails to properly update main range when subregister def moved across another subregister def

[Quuxplusone]

Bugzilla Link	PR43467
Status	RESOLVED FIXED
Importance	P normal
Reported by	Matt Arsenault (Matthew.Arsenault@amd.com)
Reported on	2019-09-26 09:02:03 -0700
Last modified on	2019-10-18 16:25:43 -0700
Version	trunk
Hardware	PC All
CC	llvm-bugs@lists.llvm.org, quentin.colombet@gmail.com, tpr.ll@botech.co.uk
Fixed by commit(s)
Attachments	0001-Add-failing-LiveIntervals-handleMoveUp-testcases.patch (6855 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 22579
Patch adding failing tests

./unittests/MI/MITests --gtest_filter=LiveIntervalTest.TestMoveSubRegNotCovered
********** INTERVALS **********
%1 [16r,32B:1)[32B,56r:3)[56r,64r:0)[80r,144B:2)  0@56r 1@16r 2@80r 3@32B-phi
L00000001 [16r,32B:1)[32B,64r:2)[80r,144B:0)  0@80r 1@16r 2@32B-phi L00000002
[16r,16d:1)[56r,112r:0)  0@56r 1@16r weight:0.000000e+00
%2 [48r,80r:0)  0@48r weight:0.000000e+00
%3 [64r,64d:0)  0@64r weight:0.000000e+00
RegMasks:
********** MACHINEINSTRS **********
# Machine code for function func: NoPHIs

0B  bb.0:
      successors: %bb.1(0x80000000); %bb.1(100.00%)

16B   %1:vreg_64 = IMPLICIT_DEF

32B bb.1:
    ; predecessors: %bb.0, %bb.1
      successors: %bb.1(0x80000000); %bb.1(100.00%)

48B   %2:vgpr_32 = V_MOV_B32_e32 2, implicit $exec
56B   %1.sub1:vreg_64 = COPY %2:vgpr_32
64B   dead %3:vgpr_32 = V_ADD_U32_e32 %2:vgpr_32, %1.sub0:vreg_64, implicit
$exec
80B   %1.sub0:vreg_64 = V_ADD_U32_e32 %2:vgpr_32, %2:vgpr_32, implicit $exec
112B      S_NOP 0, implicit %1.sub1:vreg_64
128B      S_BRANCH %bb.1

# End machine code for function func.

*** Bad machine code: A Subrange is not covered by the main range ***
- function:    func
- interval:    %1 [16r,32B:1)[32B,56r:3)[56r,64r:0)[80r,144B:2)  0@56r 1@16r
2@80r 3@32B-phi L00000001 [16r,32B:1)[32B,64r:2)[80r,144B:0)  0@80r 1@16r 2@32B-
phi L00000002 [16r,16d:1)[56r,112r:0)  0@56r 1@16r weight:0.000000e+00
LLVM ERROR: Found 1 machine code errors.

Attached is a patch adding the original test case, and 2 variants of reduced
unit tests.

This subreg def is moved up across another subregister use and def.
    %1.sub1:vreg_64 = COPY %2

The computed main range is incorrect at [56r,64r:0). This ends an instruction
too early, and should reach 80r since the def in the second add counts as a use

[Quuxplusone]

Attached 0001-Add-failing-LiveIntervals-handleMoveUp-testcases.patch (6855 bytes, text/plain): Patch adding failing tests

[Quuxplusone]

The incorrect endpoint is seemingly created here at *NewSegment. I'm pretty
confused by the usage of the next segment. After the copy_bacwkards, Next is an
identical copy of the segment at NewIdxIn.

         // OldIdxIn and OldIdxVNI are now undef and can be overridden.
          // We Slide [NewIdxIn, OldIdxIn) down one position.
          //    |- X0/NewIdxIn -| ... |- Xn-1 -||- Xn/OldIdxIn -||- OldIdxOut -|
          // => |- undef/NexIdxIn -| |- X0 -| ... |- Xn-1 -| |- Xn/OldIdxOut -|
          std::copy_backward(NewIdxIn, OldIdxIn, OldIdxOut);
          // NewIdxIn is now considered undef so we can reuse it for the moved
          // value.
          LiveRange::iterator NewSegment = NewIdxIn;
          LiveRange::iterator Next = std::next(NewSegment);
          if (SlotIndex::isEarlierInstr(Next->start, NewIdx)) {
            // There is no gap between NewSegment and its predecessor.
            *NewSegment = LiveRange::Segment(Next->start, SplitPos,
                                             Next->valno);
            *Next = LiveRange::Segment(SplitPos, Next->end, OldIdxVNI);
            Next->valno->def = SplitPos;
          } else {

[Quuxplusone]

I think this is the same problem as solved in r326087, except the def isn't dead. I'm not sure why the def being dead matters here.

If I force using the path added in r326087, it fixes the two unit tests. However, this breaks a few other cases and the original MIR testcase still errors.

[Quuxplusone]

(In reply to Matt Arsenault from comment #2)
> If I force using the path added in r326087, it fixes the two unit tests.
> However, this breaks a few other cases and the original MIR testcase still
> errors.

On closer inspection, the verifier error passes but the updated interval looks
incorrect. The live range doesn't reach the right kill instruction

[Quuxplusone]

https://reviews.llvm.org/D68149 fixes these cases

[Quuxplusone]

r375300