SplitKit: Don't further split subrange mask in buildCopy

[ruiling]

Summary

We may use several COPY instructions to copy the needed sub-registers
during split. But the way we split the lanes during the COPYs may be
different from the subranges of the old register. This would fail when we
extend the subranges of the new register because the LaneMasks do not
match exactly between subranges of new register and old register.
Since we are bundling the COPYs, I think there is no need to further refine the
subranges of the new register based on the set of LaneMasks of the inserted COPYs.

I am not sure if there will be further breaking cases. But as the subranges of
new register are created based on the LaneMasks of the subranges of old register,
it will be highly possible we will always find an exact LaneMask match.
We can think about how to make the extendPHIKillRanges() work for
subrange mask mismatch case if we meet more such cases in the future.

The test case was from D105065 by @arsenm.

[arsenm]

I'm currently working on yet another patch for this which also apparently works. With this patch, can you delete the subrange handling in extendPHIKillRanges?

[arsenm]

This patch does make much more sense to me than the other 2

[ruiling]

In D107829#2937013, @arsenm wrote:

I'm currently working on yet another patch for this which also apparently works. With this patch, can you delete the subrange handling in extendPHIKillRanges?

Do you mean completely removing the code in the loop for (LiveInterval::SubRange &PS : ParentLI.subranges()) in SplitEditor::extendPHIKillRanges()? I am not sure about this, I may need some further investigation.

[ruiling]

In D107829#2937029, @ruiling wrote:

In D107829#2937013, @arsenm wrote:

I'm currently working on yet another patch for this which also apparently works. With this patch, can you delete the subrange handling in extendPHIKillRanges?

Do you mean completely removing the code in the loop for (LiveInterval::SubRange &PS : ParentLI.subranges()) in SplitEditor::extendPHIKillRanges()? I am not sure about this, I may need some further investigation.

I am not sure why do you think of removing that piece of code? That is used to correctly update the subranges. Did I misunderstand your point?

[arsenm]

In D107829#2937029, @ruiling wrote:

In D107829#2937013, @arsenm wrote:

I'm currently working on yet another patch for this which also apparently works. With this patch, can you delete the subrange handling in extendPHIKillRanges?

Do you mean completely removing the code in the loop for (LiveInterval::SubRange &PS : ParentLI.subranges()) in SplitEditor::extendPHIKillRanges()? I am not sure about this, I may need some further investigation.

Yes. I'm able to get every test to pass by removing it, and avoiding adding dead subrange defs if the incoming value is a phi def (although I still don't trust this is always correct)

This patch LGTM. Refining subranges after all the copies are inserted makes way more sense. Figuring out why extendPHIKillRanges needs to touch the subranges is another question

[ruiling]

I am going to wait a few days to see if @qcolombet or @kparzysz has any further comments since they all followed D105065.

[qcolombet]

Looks good to me as well.

Doing the refining after all the copies have been inserted sounds much more reasonable.

llvm/lib/CodeGen/SplitKit.cpp

SlotIndex SplitEditor::buildSingleSubRegCopy(Register FromReg, Register ToReg,
     unsigned SubIdx, LiveInterval &DestLI, bool Late, SlotIndex Def) {
   const MCInstrDesc &Desc = TII.get(TargetOpcode::COPY);
   bool FirstCopy = !Def.isValid();
   MachineInstr *CopyMI = BuildMI(MBB, InsertBefore, DebugLoc(), Desc)
       .addReg(ToReg, RegState::Define | getUndefRegState(FirstCopy)
               | getInternalReadRegState(!FirstCopy), SubIdx)
       .addReg(FromReg, 0, SubIdx);
 
-  BumpPtrAllocator &Allocator = LIS.getVNInfoAllocator();
   SlotIndexes &Indexes = *LIS.getSlotIndexes();
   if (FirstCopy) {
     Def = Indexes.insertMachineInstrInMaps(*CopyMI, Late).getRegSlot();
   } else {
     CopyMI->bundleWithPred();
   }
-  LaneBitmask LaneMask = TRI.getSubRegIndexLaneMask(SubIdx);
-  DestLI.refineSubRanges(Allocator, LaneMask,
-                         [Def, &Allocator](LiveInterval::SubRange &SR) {
-                           SR.createDeadDef(Def, Allocator);
-                         },
-                         Indexes, TRI);
   return Def;
 }
 
 SlotIndex SplitEditor::buildCopy(Register FromReg, Register ToReg,
     LaneBitmask LaneMask, MachineBasicBlock &MBB,
     MachineBasicBlock::iterator InsertBefore, bool Late, unsigned RegIdx) {
   const MCInstrDesc &Desc = TII.get(TargetOpcode::COPY);
+  SlotIndexes &Indexes = *LIS.getSlotIndexes();
   if (LaneMask.all() || LaneMask == MRI.getMaxLaneMaskForVReg(FromReg)) {
     // The full vreg is copied.
     MachineInstr *CopyMI =
         BuildMI(MBB, InsertBefore, DebugLoc(), Desc, ToReg).addReg(FromReg);
-    SlotIndexes &Indexes = *LIS.getSlotIndexes();
     return Indexes.insertMachineInstrInMaps(*CopyMI, Late).getRegSlot();
   }
 
   // Only a subset of lanes needs to be copied. The following is a simple
   // heuristic to construct a sequence of COPYs. We could add a target
   // specific callback if this turns out to be suboptimal.
   LiveInterval &DestLI = LIS.getInterval(Edit->get(RegIdx));
 
   // First pass: Try to find a perfectly matching subregister index. If none
   // exists find the one covering the most lanemask bits.
   const TargetRegisterClass *RC = MRI.getRegClass(FromReg);
   assert(RC == MRI.getRegClass(ToReg) && "Should have same reg class");
 
-  SmallVector<unsigned, 8> Indexes;
+  SmallVector<unsigned, 8> SubIndexes;
 
   // Abort if we cannot possibly implement the COPY with the given indexes.
-  if (!TRI.getCoveringSubRegIndexes(MRI, RC, LaneMask, Indexes))
+  if (!TRI.getCoveringSubRegIndexes(MRI, RC, LaneMask, SubIndexes))
     report_fatal_error("Impossible to implement partial COPY");
 
   SlotIndex Def;
-  for (unsigned BestIdx : Indexes) {
+  for (unsigned BestIdx : SubIndexes) {
     Def = buildSingleSubRegCopy(FromReg, ToReg, MBB, InsertBefore, BestIdx,
                                 DestLI, Late, Def);
   }
 
+  BumpPtrAllocator &Allocator = LIS.getVNInfoAllocator();
+  DestLI.refineSubRanges(
+      Allocator, LaneMask,
+      [Def, &Allocator](LiveInterval::SubRange &SR) {
+        SR.createDeadDef(Def, Allocator);
+      },
+      Indexes, TRI);
+
   return Def;
 }
 
 VNInfo *SplitEditor::defFromParent(unsigned RegIdx,
                                    VNInfo *ParentVNI,
                                    SlotIndex UseIdx,
                                    MachineBasicBlock &MBB,
                                    MachineBasicBlock::iterator I) {

llvm/test/CodeGen/AMDGPU/extend-phi-subrange-not-in-parent.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -verify-regalloc -run-pass=greedy -o - %s | FileCheck %s
+
+# Initially %2 starts out with 2 subranges (one for sub0, and one for
+# the rest of the lanes). After %2 is split, after refineSubRanges the
+# newly created register has a different set of lane masks since the
+# copy bundle uses 2 different defs to cover the register. This was
+# fixed by doing refineSubRanges after all the COPYs being inserted.
+
+---
+name: subrange_for_this_mask_not_found
+tracksRegLiveness: true
+machineFunctionInfo:
+  scratchRSrcReg:  '$sgpr0_sgpr1_sgpr2_sgpr3'
+  stackPtrOffsetReg: '$sgpr32'
+  occupancy:       7
+body:             |
+  ; CHECK-LABEL: name: subrange_for_this_mask_not_found
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.1(0x80000000)
+  ; CHECK:   [[DEF:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
+  ; CHECK:   [[DEF1:%[0-9]+]]:vreg_1024_align2 = IMPLICIT_DEF
+  ; CHECK:   SI_SPILL_V1024_SAVE [[DEF1]], %stack.0, $sgpr32, 0, implicit $exec :: (store (s1024) into %stack.0, align 4, addrspace 5)
+  ; CHECK: bb.1:
+  ; CHECK:   successors: %bb.1(0x40000000), %bb.2(0x40000000)
+  ; CHECK:   S_NOP 0, implicit [[DEF1]]
+  ; CHECK:   S_NOP 0, implicit [[DEF1]]
+  ; CHECK:   [[DEF2:%[0-9]+]]:vreg_1024_align2 = IMPLICIT_DEF
+  ; CHECK:   S_CBRANCH_VCCNZ %bb.1, implicit undef $vcc
+  ; CHECK: bb.2:
+  ; CHECK:   successors: %bb.3(0x80000000)
+  ; CHECK:   [[SI_SPILL_V1024_RESTORE:%[0-9]+]]:vreg_1024_align2 = SI_SPILL_V1024_RESTORE %stack.0, $sgpr32, 0, implicit $exec :: (load (s1024) from %stack.0, align 4, addrspace 5)
+  ; CHECK:   undef %5.sub1_sub2_sub3_sub4_sub5_sub6_sub7_sub8_sub9_sub10_sub11_sub12_sub13_sub14_sub15_sub16:vreg_1024_align2 = COPY [[SI_SPILL_V1024_RESTORE]].sub1_sub2_sub3_sub4_sub5_sub6_sub7_sub8_sub9_sub10_sub11_sub12_sub13_sub14_sub15_sub16 {
+  ; CHECK:     internal %5.sub16_sub17_sub18_sub19_sub20_sub21_sub22_sub23_sub24_sub25_sub26_sub27_sub28_sub29_sub30_sub31:vreg_1024_align2 = COPY [[SI_SPILL_V1024_RESTORE]].sub16_sub17_sub18_sub19_sub20_sub21_sub22_sub23_sub24_sub25_sub26_sub27_sub28_sub29_sub30_sub31
+  ; CHECK:   }
+  ; CHECK:   %5.sub0:vreg_1024_align2 = IMPLICIT_DEF
+  ; CHECK:   S_NOP 0, implicit %5.sub0
+  ; CHECK: bb.3:
+  ; CHECK:   successors: %bb.4(0x80000000)
+  ; CHECK:   S_NOP 0, implicit %5
+  ; CHECK: bb.4:
+  ; CHECK:   successors: %bb.3(0x40000000), %bb.5(0x40000000)
+  ; CHECK:   [[DEF2:%[0-9]+]]:vreg_1024_align2 = IMPLICIT_DEF
+  ; CHECK:   S_CBRANCH_VCCNZ %bb.3, implicit undef $vcc
+  ; CHECK: bb.5:
+  ; CHECK:   undef %3.sub0:vreg_1024_align2 = COPY [[DEF]]
+  ; CHECK:   S_NOP 0, implicit %3
+  bb.0:
+    %0:vgpr_32 = IMPLICIT_DEF
+    %1:vreg_1024_align2 = IMPLICIT_DEF
+    %2:vreg_1024_align2 = COPY %1
+
+  bb.1:
+    S_NOP 0, implicit %1
+    S_NOP 0, implicit %1
+    %1:vreg_1024_align2 = IMPLICIT_DEF
+    S_CBRANCH_VCCNZ %bb.1, implicit undef $vcc
+
+  bb.3:
+    %2.sub0:vreg_1024_align2 = IMPLICIT_DEF
+    S_NOP 0, implicit %2.sub0
+
+  bb.4:
+    S_NOP 0, implicit %2
+
+  bb.5:
+    %2:vreg_1024_align2 = IMPLICIT_DEF
+    S_CBRANCH_VCCNZ %bb.4, implicit undef $vcc
+
+  bb.6:
+    undef %4.sub0:vreg_1024_align2 = COPY %0
+    S_NOP 0, implicit %4
+...