Replace subregister uses when processing tied operands

[arsenm]

Summary

This was for some reason skipping operands that are subregisters
instead of keeping the same subregister index.

v_movreld_b32 expects src0 to be the subregister of the tied
super register use/def.

e.g.

v_movreld_b32 v0, v9, <imp-def, tied3> v[0:3], <imp-use, tied2> v[0:3]

was being replaced with

v[4:7] = copy v[0:3]
v_movreld_b32 v0, v9, <imp-def, tied3> v[4:7], <imp-use, tied2> v[4:7],

which really writes to v[0:3]

[nhaehnle]

This looks good to me, but it also needs to cover the "rewrite undef" path, see the test case at https://lists.freedesktop.org/archives/mesa-dev/2016-July/124285.html

[nhaehnle]

What's the status of this change? I just ran into this bug again...

[nhaehnle]

Please see http://paste.ubuntu.com/22949830/ for a test case that gets broken by this.

The issue is that

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg3:sub3, %vreg3:sub2<tied0>

becomes

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg8:sub3, %vreg8<tied0>

However, something like this change is still required to fix other bugs related to v_morveld/s.

[arsenm]

There was already code there trying to accomplish the same thing, it was just skipping for subregisters for some reason

[arsenm]

In D22556#511686, @nhaehnle wrote:

Please see http://paste.ubuntu.com/22949830/ for a test case that gets broken by this.

The issue is that

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg3:sub3, %vreg3:sub2<tied0>

becomes

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg8:sub3, %vreg8<tied0>

However, something like this change is still required to fix other bugs related to v_morveld/s.

This testcase and another related one I found are still broken with this, but I don't

In D22556#511686, @nhaehnle wrote:

Please see http://paste.ubuntu.com/22949830/ for a test case that gets broken by this.

The issue is that

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg3:sub3, %vreg3:sub2<tied0>

becomes

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg8:sub3, %vreg8<tied0>

However, something like this change is still required to fix other bugs related to v_morveld/s.

I think the problem with this testcase might instead be SIFoldOperands producing the subreg use on the tied operand. I'm not sure if this necessarily makes sense:
%vreg15<def,tied6> = V_MAC_F32_e64 0, %vreg14<kill>, 0, %vreg11:sub3, 0, %vreg11:sub2<tied0>, 0, 0, %EXEC<imp-use>; VGPR_32:%vreg15,%vreg14 VReg_128:%vreg11

[arsenm]

In D22556#514600, @arsenm wrote:

In D22556#511686, @nhaehnle wrote:

Please see http://paste.ubuntu.com/22949830/ for a test case that gets broken by this.

The issue is that

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg3:sub3, %vreg3:sub2<tied0>

becomes

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg8:sub3, %vreg8<tied0>

However, something like this change is still required to fix other bugs related to v_morveld/s.

This testcase and another related one I found are still broken with this, but I don't

In D22556#511686, @nhaehnle wrote:

Please see http://paste.ubuntu.com/22949830/ for a test case that gets broken by this.

The issue is that

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg3:sub3, %vreg3:sub2<tied0>

becomes

%vreg8<def,tied3> = V_MAC_F32_e32 1132462080, %vreg8:sub3, %vreg8<tied0>

However, something like this change is still required to fix other bugs related to v_morveld/s.

I think the problem with this testcase might instead be SIFoldOperands producing the subreg use on the tied operand. I'm not sure if this necessarily makes sense:
%vreg15<def,tied6> = V_MAC_F32_e64 0, %vreg14<kill>, 0, %vreg11:sub3, 0, %vreg11:sub2<tied0>, 0, 0, %EXEC<imp-use>; VGPR_32:%vreg15,%vreg14 VReg_128:%vreg11

D23482 solves this. I wonder if it should be a verifier error to have an operand tied with another with different sub reg indices

[arsenm]

ping

[arsenm]

ping

[MatzeB]

LGTM.

[arsenm]

r279804

lib/CodeGen/TwoAddressInstructionPass.cpp

if (SrcMO.isUndef() && !DstMO.getSubReg()) {
       DEBUG(dbgs() << "\t\trewrite undef:\t" << *MI);
       continue;
     }
     TiedOperands[SrcReg].push_back(std::make_pair(SrcIdx, DstIdx));
   }
   return AnyOps;
 }
 
+static void substituteRegisterUses(MachineRegisterInfo &MRI,
+                                   MachineInstr &MI,
+                                   unsigned OldReg, unsigned NewReg) {
+  for (MachineOperand &MO : MI.operands()) {
+    // Skip registers that are tied to another operand.
+    if (MO.isReg() && !MO.isTied() && MO.getReg() == OldReg)
+      MO.setReg(NewReg);
+  }
+}
+
 // Process a list of tied MI operands that all use the same source register.
 // The tied pairs are of the form (SrcIdx, DstIdx).
 void
 TwoAddressInstructionPass::processTiedPairs(MachineInstr *MI,
                                             TiedPairList &TiedPairs,
                                             unsigned &Dist) {
   bool IsEarlyClobber = false;
   for (unsigned tpi = 0, tpe = TiedPairs.size(); tpi != tpe; ++tpi) {
if (TargetRegisterInfo::isVirtualRegister(RegA) &&
         TargetRegisterInfo::isVirtualRegister(RegB))
       MRI->constrainRegClass(RegA, RC);
     MO.setReg(RegA);
     // The getMatchingSuper asserts guarantee that the register class projected
     // by SubRegB is compatible with RegA with no subregister. So regardless of
     // whether the dest oper writes a subreg, the source oper should not.
     MO.setSubReg(0);
 
+    if (!DstMO.isEarlyClobber())
+      substituteRegisterUses(*MRI, *MI, RegB, RegA);
+
     // Propagate SrcRegMap.
     SrcRegMap[RegA] = RegB;
   }
 
   if (AllUsesCopied) {
     if (!IsEarlyClobber) {
       // Replace other (un-tied) uses of regB with LastCopiedReg.
       for (MachineOperand &MO : MI->operands()) {

test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll

+; RUN: llc -O0 -march=amdgcn -verify-machineinstrs < %s | FileCheck %s
+
+; FIXME: Merge into indirect-addressing-si.ll
+
+; Make sure that TwoAddressInstructions keeps src0 as subregister sub0
+; of the tied implicit use and def of the super register.
+
+; CHECK-LABEL: {{^}}insert_wo_offset:
+; CHECK: s_load_dword [[IN:s[0-9]+]]
+; CHECK: s_mov_b32 m0, [[IN]]
+; CHECK: v_movreld_b32_e32 v[[ELT0:[0-9]+]]
+; CHECK-NEXT: buffer_store_dwordx4 v{{\[}}[[ELT0]]:
+define void @insert_wo_offset(<4 x float> addrspace(1)* %out, i32 %in) {
+entry:
+  %ins = insertelement <4 x float> <float 1.0, float 2.0, float 3.0, float 4.0>, float 5.0, i32 %in
+  store <4 x float> %ins, <4 x float> addrspace(1)* %out
+  ret void
+}
+

test/CodeGen/AMDGPU/indirect-addressing-si.ll

 entry:
   %ld = load <4 x i32>, <4  x i32> addrspace(1)* %in
   %value = insertelement <4 x i32> %ld, i32 5, i32 undef
   store <4 x i32> %value, <4 x i32> addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}insert_w_offset:
-; CHECK: s_load_dword [[IN:s[0-9]+]]
-; CHECK: s_mov_b32 m0, [[IN]]
-; CHECK: v_movreld_b32_e32
-define void @insert_w_offset(float addrspace(1)* %out, i32 %in) {
+; CHECK-DAG: s_load_dword [[IN:s[0-9]+]]
+; CHECK-DAG: s_mov_b32 m0, [[IN]]
+; CHECK-DAG: v_mov_b32_e32 v[[ELT0:[0-9]+]], 1.0
+; CHECK-DAG: v_mov_b32_e32 v[[ELT1:[0-9]+]], 2.0
+; CHECK-DAG: v_mov_b32_e32 v[[ELT2:[0-9]+]], 0x40400000
+; CHECK-DAG: v_mov_b32_e32 v[[ELT3:[0-9]+]], 4.0
+; CHECK-DAG: v_mov_b32_e32 v[[INS:[0-9]+]], 0x40a00000
+; CHECK: v_movreld_b32_e32 v[[ELT1]], v[[INS]]
+; CHECK: buffer_store_dwordx4 v{{\[}}[[ELT0]]:[[ELT3]]{{\]}}
+define void @insert_w_offset(<4 x float> addrspace(1)* %out, i32 %in) {
 entry:
   %0 = add i32 %in, 1
   %1 = insertelement <4 x float> <float 1.0, float 2.0, float 3.0, float 4.0>, float 5.0, i32 %0
-  %2 = extractelement <4 x float> %1, i32 2
-  store float %2, float addrspace(1)* %out
+  store <4 x float> %1, <4 x float> addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}insert_wo_offset:
 ; CHECK: s_load_dword [[IN:s[0-9]+]]
 ; CHECK: s_mov_b32 m0, [[IN]]
-; CHECK: v_movreld_b32_e32
-define void @insert_wo_offset(float addrspace(1)* %out, i32 %in) {
+; CHECK: v_movreld_b32_e32 v[[ELT0:[0-9]+]]
+; CHECK: buffer_store_dwordx4 v{{\[}}[[ELT0]]:
+define void @insert_wo_offset(<4 x float> addrspace(1)* %out, i32 %in) {
 entry:
   %0 = insertelement <4 x float> <float 1.0, float 2.0, float 3.0, float 4.0>, float 5.0, i32 %in
-  %1 = extractelement <4 x float> %0, i32 2
-  store float %1, float addrspace(1)* %out
+  store <4 x float> %0, <4 x float> addrspace(1)* %out
   ret void
 }
 
 ; CHECK-LABEL: {{^}}insert_neg_offset_sgpr:
 ; The offset depends on the register that holds the first element of the vector.
 ; CHECK: s_add_i32 m0, s{{[0-9]+}}, 0xfffffe{{[0-9a-z]+}}
 ; CHECK: v_movreld_b32_e32 v0, 5
 define void @insert_neg_offset_sgpr(i32 addrspace(1)* %in, <4 x i32> addrspace(1)* %out, i32 %offset) {