From 5da784840c5f773e21eb224ee5887e82df7b6d50 Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Mon, 11 Sep 2023 00:44:33 +0100 Subject: [PATCH 1/3] [TwoAddressInstruction] Check if segment was found when updating subreg 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. --- .../lib/CodeGen/TwoAddressInstructionPass.cpp | 5 +++-- ...ressinstruction-subreg-liveness-update.mir | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir diff --git a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp index 45f61262faf93..6ceb4b43522f0 100644 --- a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp +++ b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp @@ -1868,12 +1868,13 @@ bool TwoAddressInstructionPass::runOnMachineFunction(MachineFunction &Func) { // %reg.subidx. LaneBitmask LaneMask = TRI->getSubRegIndexLaneMask(mi->getOperand(0).getSubReg()); - SlotIndex Idx = LIS->getInstructionIndex(*mi); + SlotIndex Idx = LIS->getInstructionIndex(*mi).getRegSlot(); for (auto &S : LI.subranges()) { if ((S.LaneMask & LaneMask).none()) { LiveRange::iterator UseSeg = S.FindSegmentContaining(Idx); LiveRange::iterator DefSeg = std::next(UseSeg); - S.MergeValueNumberInto(DefSeg->valno, UseSeg->valno); + if (DefSeg != S.end()) + S.MergeValueNumberInto(DefSeg->valno, UseSeg->valno); } } diff --git a/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir b/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir new file mode 100644 index 0000000000000..abafc580a968f --- /dev/null +++ b/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir @@ -0,0 +1,22 @@ +# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3 +# RUN: llc %s -o - -mtriple=riscv64 -mattr=+v -run-pass=liveintervals,twoaddressinstruction,register-coalescer | FileCheck %s + +... +--- +name: insert_v2i64_nxv16i64 +tracksRegLiveness: true +body: | + bb.0: + liveins: $v8 + ; CHECK-LABEL: name: insert_v2i64_nxv16i64 + ; CHECK: liveins: $v8 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: KILL $v8 + ; CHECK-NEXT: PseudoRET + %0:vr = COPY killed $v8 + %2:vrm8 = INSERT_SUBREG undef %1:vrm8, killed %0, %subreg.sub_vrm1_0 + %3:vrm4 = COPY %2.sub_vrm4_0:vrm8 + + PseudoRET + +... From aa0c04b83f2ff9cb075f04858632688f57514925 Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Mon, 11 Sep 2023 14:40:21 +0100 Subject: [PATCH 2/3] Merge segments together if there's > 1, otherwise delete the sole segment --- .../lib/CodeGen/TwoAddressInstructionPass.cpp | 28 ++++++++++++++++--- ...ressinstruction-subreg-liveness-update.mir | 26 +++++++++++++++-- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp index 6ceb4b43522f0..3ea22eff91593 100644 --- a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp +++ b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp @@ -1868,15 +1868,35 @@ bool TwoAddressInstructionPass::runOnMachineFunction(MachineFunction &Func) { // %reg.subidx. LaneBitmask LaneMask = TRI->getSubRegIndexLaneMask(mi->getOperand(0).getSubReg()); - SlotIndex Idx = LIS->getInstructionIndex(*mi).getRegSlot(); + SlotIndex Idx = LIS->getInstructionIndex(*mi); for (auto &S : LI.subranges()) { if ((S.LaneMask & LaneMask).none()) { + // If Idx is 160B, and we have a subrange that isn't in + // %reg.subidx like so: + // + // [152r,160r)[160r,256r) + // + // Merge the two segments together so the subrange becomes: + // + // [152r,256r) LiveRange::iterator UseSeg = S.FindSegmentContaining(Idx); - LiveRange::iterator DefSeg = std::next(UseSeg); - if (DefSeg != S.end()) - S.MergeValueNumberInto(DefSeg->valno, UseSeg->valno); + if (UseSeg != S.end()) { + LiveRange::iterator DefSeg = std::next(UseSeg); + if (DefSeg != S.end()) + S.MergeValueNumberInto(DefSeg->valno, UseSeg->valno); + else + S.removeSegment(UseSeg); + } + // Otherwise, it should have only one segment that starts at + // 160r which we should remove. + else { + assert(S.containsOneValue()); + assert(S.begin()->start == Idx.getRegSlot()); + S.removeSegment(S.begin()); + } } } + LI.removeEmptySubRanges(); // The COPY no longer has a use of %reg. LIS->shrinkToUses(&LI); diff --git a/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir b/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir index abafc580a968f..03252038f03e2 100644 --- a/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir +++ b/llvm/test/CodeGen/RISCV/rvv/twoaddressinstruction-subreg-liveness-update.mir @@ -1,14 +1,14 @@ # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3 -# RUN: llc %s -o - -mtriple=riscv64 -mattr=+v -run-pass=liveintervals,twoaddressinstruction,register-coalescer | FileCheck %s +# RUN: llc %s -o - -mtriple=riscv64 -mattr=+v -run-pass=liveintervals,twoaddressinstruction,register-coalescer -verify-machineinstrs | FileCheck %s ... --- -name: insert_v2i64_nxv16i64 +name: f tracksRegLiveness: true body: | bb.0: liveins: $v8 - ; CHECK-LABEL: name: insert_v2i64_nxv16i64 + ; CHECK-LABEL: name: f ; CHECK: liveins: $v8 ; CHECK-NEXT: {{ $}} ; CHECK-NEXT: KILL $v8 @@ -20,3 +20,23 @@ body: | PseudoRET ... +--- +name: g +tracksRegLiveness: true +body: | + bb.0: + liveins: $v8, $v16 + ; CHECK-LABEL: name: g + ; CHECK: liveins: $v8, $v16 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: KILL $v8 + ; CHECK-NEXT: dead [[COPY:%[0-9]+]]:vrm8 = COPY $v16 + ; CHECK-NEXT: PseudoRET + %0:vr = COPY killed $v8 + %1:vrm8 = COPY killed $v16 + %2:vrm8 = INSERT_SUBREG %1:vrm8, killed %0, %subreg.sub_vrm1_0 + %3:vrm4 = COPY %2.sub_vrm4_0:vrm8 + + PseudoRET + +... From 9553f33adcf010ccf371776babb6f99751d533e7 Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Mon, 11 Sep 2023 14:47:00 +0100 Subject: [PATCH 3/3] Change to assert that if we find a segment at the instruction index, it should always have another segment after it --- llvm/lib/CodeGen/TwoAddressInstructionPass.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp index 3ea22eff91593..e36bffc91b91d 100644 --- a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp +++ b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp @@ -1882,10 +1882,8 @@ bool TwoAddressInstructionPass::runOnMachineFunction(MachineFunction &Func) { LiveRange::iterator UseSeg = S.FindSegmentContaining(Idx); if (UseSeg != S.end()) { LiveRange::iterator DefSeg = std::next(UseSeg); - if (DefSeg != S.end()) - S.MergeValueNumberInto(DefSeg->valno, UseSeg->valno); - else - S.removeSegment(UseSeg); + assert(DefSeg != S.end()); + S.MergeValueNumberInto(DefSeg->valno, UseSeg->valno); } // Otherwise, it should have only one segment that starts at // 160r which we should remove.