[TwoAddressInstruction] Check if segment was found when updating subreg LIs #65916

lukel97 · 2023-09-10T23:52:58Z

This patch fixes a crash I was seeing when working with subregisters in a
separate upcoming patch. Some of the RVV tests have -early-live-intervals
enabled, which triggers this subregister liveness updating code in
TwoAddressInstruction.

The INSERT_SUBREG in the test case gets converted to a COPY, so it tries to
update the subranges for %1 below:

********** INTERVALS **********
V8 [0B,16r:0) 0@0B-phi
%0 [16r,32r:0) 0@16r weight:0.000000e+00
%1 [32r,48r:0) 0@32r L000000000000003C [32r,48r:0) 0@32r L00000000000003C0 [32r,32d:0) 0@32r weight:0.000000e+00
%2 EMPTY weight:0.000000e+00
%3 [48r,48d:0) 0@48r weight:0.000000e+00

0B bb.0:
liveins: $v8
16B %0:vr = COPY $v8
32B undef %1.sub_vrm1_0:vrm8 = COPY %0:vr
48B dead %3:vrm4 = COPY %1.sub_vrm4_0:vrm8
64B PseudoRET

The subrange for %1 L00000000000003C0 [32r,32d:0) doesn't overlap with
sub_vrm1_0's lane mask, so it tries to merge 0@32r. It calls
FindSegmentContaining(32B), 32B being the instruction index for the
INSERT_SUBREG-turned-COPY. But 32B isn't actually in [32r,32d:0) so it returns
end(). The code wasn't checking for end() though so it crashed when
dereferencing DefSeg.

I'm not familiar with this area, but my guess is that we need to handle the case where a subrange only has one segment, e.g. where there were no previous defs before the insert_subregister.

I also did a quick check, nothing in llvm-check-codegen seems to trigger this
code. Not sure if that will change if -early-live-intervals is enabled by
default.

…eg LIs This patch fixes a crash I was seeing when working with subregisters in a separate upcoming patch. Some of the RVV tests have -early-live-intervals enabled, which triggers this subregister liveness updating code in TwoAddressInstruction. The INSERT_SUBREG in the test case gets converted to a COPY, so it tries to update the subranges for %1 below: ********** INTERVALS ********** V8 [0B,16r:0) 0@0B-phi %0 [16r,32r:0) 0@16r weight:0.000000e+00 %1 [32r,48r:0) 0@32r L000000000000003C [32r,48r:0) 0@32r L00000000000003C0 [32r,32d:0) 0@32r weight:0.000000e+00 %2 EMPTY weight:0.000000e+00 %3 [48r,48d:0) 0@48r weight:0.000000e+00 0B bb.0: liveins: $v8 16B %0:vr = COPY $v8 32B undef %1.sub_vrm1_0:vrm8 = COPY %0:vr 48B dead %3:vrm4 = COPY %1.sub_vrm4_0:vrm8 64B PseudoRET The subrange for %1 L00000000000003C0 [32r,32d:0) doesn't overlap with sub_vrm1_0's lane mask, so it tries to merge 0@32r. It calls FindSegmentContaining(32B), 32B being the instruction index for the INSERT_SUBREG-turned-COPY. But 32B isn't actually in [32r,32d:0) so it returns end(). The code wasn't checking for end() though so it crashed when dereferencing DefSeg. I'm not familiar with this area in the slightest, but I presume we wanted to find the segment at 32r? And in this test case there's no other segment in the subrange anyway, so we need to check if DefSeg exists. I also did a quick check, nothing in llvm-check-codegen seems to trigger this code. Not sure if that will change if -early-live-intervals is enabled by default.

BeMg · 2023-09-11T05:50:47Z

Hi @lukel97

I do some experiment which recompute the LiveInterval even it has subrange inside TwoAddressInstructionPass pass.

// Orgin LiveIntervals before updating LiveIntervals.
********** INTERVALS **********
V8 [0B,16r:0) 0@0B-phi
%0 [16r,32r:0) 0@16r  weight:0.000000e+00
%1 [32r,48r:0) 0@32r  L000000000000003C [32r,48r:0) 0@32r  L00000000000003C0 [32r,32d:0) 0@32r  weight:0.000000e+00
%2 EMPTY  weight:0.000000e+00
%3 [48r,48d:0) 0@48r  weight:0.000000e+00
RegMasks:
********** MACHINEINSTRS **********
# Machine code for function insert_v2i64_nxv16i64: NoPHIs, TracksLiveness, TiedOpsRewritten

0B      bb.0:
          liveins: $v8
16B       %0:vr = COPY $v8
32B       undef %1.sub_vrm1_0:vrm8 = COPY %0:vr
48B       dead %3:vrm4 = COPY %1.sub_vrm4_0:vrm8
64B       PseudoRET

# End machine code for function insert_v2i64_nxv16i64.

// Update LiveIntervals with Recompute by LIS's member function
// Something like 
// LIS->removeInterval(Reg);
// LIS->createAndComputeVirtRegInterval(Reg);
********** INTERVALS **********
V8 [0B,16r:0) 0@0B-phi
%0 [16r,32r:0) 0@16r  weight:0.000000e+00
%1 [32r,48r:0) 0@32r  L0000000000000004 [32r,48r:0) 0@32r  weight:0.000000e+00
%2 EMPTY  weight:0.000000e+00
%3 [48r,48d:0) 0@48r  weight:0.000000e+00
RegMasks:
********** MACHINEINSTRS **********
# Machine code for function insert_v2i64_nxv16i64: NoPHIs, TracksLiveness, TiedOpsRewritten

0B      bb.0:
          liveins: $v8
16B       %0:vr = COPY $v8
32B       undef %1.sub_vrm1_0:vrm8 = COPY %0:vr
48B       dead %3:vrm4 = COPY %1.sub_vrm4_0:vrm8
64B       PseudoRET

# End machine code for function insert_v2i64_nxv16i64.

// Update LiveIntervals by your patch
********** INTERVALS **********
V8 [0B,16r:0) 0@0B-phi
%0 [16r,32r:0) 0@16r  weight:0.000000e+00
%1 [32r,48r:0) 0@32r  L000000000000003C [32r,48r:0) 0@32r  L00000000000003C0 [32r,32d:0) 0@32r  weight:0.000000e+00
%2 EMPTY  weight:0.000000e+00
%3 [48r,48d:0) 0@48r  weight:0.000000e+00
RegMasks:
********** MACHINEINSTRS **********
# Machine code for function insert_v2i64_nxv16i64: NoPHIs, TracksLiveness, TiedOpsRewritten

0B      bb.0:
          liveins: $v8
16B       %0:vr = COPY $v8
32B       undef %1.sub_vrm1_0:vrm8 = COPY %0:vr
48B       dead %3:vrm4 = COPY %1.sub_vrm4_0:vrm8
64B       PseudoRET

# End machine code for function insert_v2i64_nxv16i64.

Should they achieve the same result?

lukel97 · 2023-09-11T12:51:47Z

@BeMg Thanks for running that, I think they should be the same. The only subrange in %1 should be sub_vrm1_0 right? I believe that's the lane mask L0000000000000004. Looks the subrange L00000000000003C0 should be removed in this test case. Will take a look

…ment

…it should always have another segment after it

Similar to llvm#65598, if we're using a vslideup to insert a fixed length vector into another vector, then we can work out the minimum number of registers it will need to slide up across given the minimum VLEN, and shrink the type operated on to reduce LMUL accordingly. This is somewhat dependent on llvm#65916, since it introduces a subregister copy that triggers a crash with -early-live-intervals in one of the tests.

lukel97 requested a review from a team as a code owner September 10, 2023 23:52

lukel97 requested a review from jayfoad September 10, 2023 23:52

kito-cheng requested a review from BeMg September 11, 2023 02:25

lukel97 added 2 commits September 11, 2023 14:40

Merge segments together if there's > 1, otherwise delete the sole seg…

aa0c04b

…ment

Change to assert that if we find a segment at the instruction index, …

9553f33

…it should always have another segment after it

lukel97 requested a review from MatzeB September 11, 2023 13:52

lukel97 mentioned this pull request Sep 11, 2023

[RISCV] Shrink vslideup's LMUL when lowering fixed insert_subvector #65997

Merged

lukel97 mentioned this pull request Sep 12, 2023

[RISCV] Reduce LMUL when index is known when lowering insert_vector_elt #66087

Merged

lukel97 closed this Sep 13, 2023

jayfoad mentioned this pull request Sep 13, 2023

[TwoAddressInstruction] Update LiveIntervals after INSERT_SUBREG with undef read #66211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwoAddressInstruction] Check if segment was found when updating subreg LIs #65916

[TwoAddressInstruction] Check if segment was found when updating subreg LIs #65916

lukel97 commented Sep 10, 2023 •

edited

BeMg commented Sep 11, 2023

lukel97 commented Sep 11, 2023

[TwoAddressInstruction] Check if segment was found when updating subreg LIs #65916

[TwoAddressInstruction] Check if segment was found when updating subreg LIs #65916

Conversation

lukel97 commented Sep 10, 2023 • edited

BeMg commented Sep 11, 2023

lukel97 commented Sep 11, 2023

lukel97 commented Sep 10, 2023 •

edited