Fix MachineInstr::findRegisterUseOperandIdx subreg checks

[rampitec]

Summary

The function only checks that instruction reads a super-register
containing requested physical register. In case if a sub-register
if being read that is also a use of a super-reg, so added the check.
In particular MI->readsRegister() is broken because of the missing
check. The resulting check is essentially regsOverlap().

[rampitec]

I have realized the old check was also needed, an instruction reads a register in both cases: if the operand is a subreg of a requested register or if it is a superreg.

With the new patch previous changes to other targets are gone. Therefor gone the ask to review their test changes.

[rampitec]

The effective check is just regsOverlap().

[arsenm]

lib/CodeGen/MachineInstr.cpp

936 ↗

(On Diff #172711)

Why is TRI optional here? I would expect it to be mandatory if there is a physical register and assert

[rampitec]

lib/CodeGen/MachineInstr.cpp

936 ↗

(On Diff #172711)

That's a good question why, but it is optional.
I have tried to use just TRI->regsOverlap() instead of the whole condition, and then run into crashes due to TRI being nullptr.

[rampitec]

lib/CodeGen/MachineInstr.cpp

936 ↗

(On Diff #172711)

Here are just some examples of places where the function is called without optional TRI operand and with a physreg:

lib/Target/AArch64/AArch64InstrInfo.cpp:

1515│ case AArch64::FCSELDrrr: {
1516├> int Idx = Instr.findRegisterUseOperandIdx(AArch64::NZCV);

lib/Target/AMDGPU/SIInstrInfo.cpp:

4769├> if (MI.findRegisterUseOperandIdx(AMDGPU::SCC) != -1)
4770│ Worklist.insert(&MI);

lib/Target/ARM/ARMLoadStoreOptimizer.cpp:

891├> bool BaseKill = LatestMI->killsRegister(Base);

and then:

include/llvm/CodeGen/MachineInstr.h:

1134│ bool killsRegister(unsigned Reg,
1135│ const TargetRegisterInfo *TRI = nullptr) const {
1136├> return findRegisterUseOperandIdx(Reg, true, TRI) != -1;

There is huge number of places across all targets.

[rampitec]

lib/CodeGen/MachineInstr.cpp

936 ↗

(On Diff #172711)

Altogether 2539 tests fail across all targets if TRI check is removed, even if physreg check is in place.

[arsenm]

lib/CodeGen/MachineInstr.cpp

936 ↗

(On Diff #172711)

Is there any real reason not to pass it in though? I would think it shouldn't be hard to make it mandatory, other than the manual work

[rampitec]

lib/CodeGen/MachineInstr.cpp

936 ↗

(On Diff #172711)

I do not think there is such a reason. That is just a lot of places across all backends.
Anyway, this is really orthogonal to this patch.

[nhaehnle]

The code change looks fine to me, but it should be possible to cleanup the test case a bit.

test/CodeGen/AMDGPU/optimize-if-exec-masking.mir

135–151 ↗

(On Diff #172711)

It should be possible to remove most of the IR here.

760–781 ↗

(On Diff #172711)

It should be possible to remove most of this boilerplate.

[rampitec]

Test cleanup.

[arsenm]

LGTM

llvm/trunk/lib/CodeGen/MachineInstr.cpp

int MachineInstr::findRegisterUseOperandIdx(
     unsigned Reg, bool isKill, const TargetRegisterInfo *TRI) const {
   for (unsigned i = 0, e = getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = getOperand(i);
     if (!MO.isReg() || !MO.isUse())
       continue;
     unsigned MOReg = MO.getReg();
     if (!MOReg)
       continue;
-    if (MOReg == Reg || (TRI && TargetRegisterInfo::isPhysicalRegister(MOReg) &&
-                         TargetRegisterInfo::isPhysicalRegister(Reg) &&
-                         TRI->isSubRegister(MOReg, Reg)))
+    if (MOReg == Reg || (TRI && Reg && MOReg && TRI->regsOverlap(MOReg, Reg)))
       if (!isKill || MO.isKill())
         return i;
   }
   return -1;
 }
 
 /// readsWritesVirtualRegister - Return a pair of bools (reads, writes)
 /// indicating if this instruction reads or writes Reg. This also considers

llvm/trunk/test/CodeGen/AMDGPU/optimize-if-exec-masking.mir

--- |
 
   if:
     br label %end
 
   end:
     ret void
   }
 
+  define amdgpu_kernel void @if_and_xor_read_exec_copy_subreg() {
+  main_body:
+    br i1 undef, label %if, label %end
+
+  if:                                               ; preds = %main_body
+    br label %end
+
+  end:                                              ; preds = %if, %main_body
+    ret void
+  }
+
 ...
 ---
 # CHECK-LABEL: name: optimize_if_and_saveexec_xor{{$}}
 # CHECK: $sgpr0_sgpr1 = S_AND_SAVEEXEC_B64 $vcc, implicit-def $exec, implicit-def $scc, implicit $exec
 # CHECK-NEXT: $sgpr0_sgpr1 = S_XOR_B64 $exec, killed $sgpr0_sgpr1, implicit-def $scc
 # CHECK-NEXT: SI_MASK_BRANCH
 
 name:            optimize_if_and_saveexec_xor
bb.2.end:
 
     $exec = S_OR_B64 $exec, killed $sgpr0_sgpr1, implicit-def $scc
     $sgpr3 = S_MOV_B32 61440
     $sgpr2 = S_MOV_B32 -1
     BUFFER_STORE_DWORD_OFFSET killed $vgpr0, $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0, 0, 0, 0, implicit $exec
     S_ENDPGM
 
 ...
+---
+# A read from exec copy subreg prevents optimization
+# CHECK-LABEL: name: if_and_xor_read_exec_copy_subreg{{$}}
+# CHECK: $sgpr0_sgpr1 = COPY $exec
+# CHECK-NEXT: $sgpr4 = S_MOV_B32 $sgpr1
+name:            if_and_xor_read_exec_copy_subreg
+liveins:
+  - { reg: '$vgpr0' }
+body:             |
+  bb.0.main_body:
+    liveins: $vgpr0
+
+    $sgpr0_sgpr1 = COPY $exec
+    $sgpr4 = S_MOV_B32 $sgpr1
+    $vcc = V_CMP_EQ_I32_e64 0, killed $vgpr0, implicit $exec
+    $vgpr0 = V_MOV_B32_e32 4, implicit $exec
+    $sgpr2_sgpr3 = S_AND_B64 $sgpr0_sgpr1, killed $vcc, implicit-def $scc
+    $sgpr0_sgpr1 = S_XOR_B64 $sgpr2_sgpr3, killed $sgpr0_sgpr1, implicit-def $scc
+    $exec = S_MOV_B64_term killed $sgpr2_sgpr3
+    SI_MASK_BRANCH %bb.2, implicit $exec
+    S_BRANCH %bb.1
+
+  bb.1.if:
+    liveins: $sgpr0_sgpr1
+
+    $sgpr7 = S_MOV_B32 61440
+    $sgpr6 = S_MOV_B32 -1
+    $vgpr0 = BUFFER_LOAD_DWORD_OFFSET $sgpr4_sgpr5_sgpr6_sgpr7, 0, 0, 0, 0, 0, implicit $exec
+
+  bb.2.end:
+    liveins: $vgpr0, $sgpr0_sgpr1
+
+    $exec = S_OR_B64 $exec, killed $sgpr0_sgpr1, implicit-def $scc
+    $sgpr3 = S_MOV_B32 61440
+    $sgpr2 = S_MOV_B32 -1
+    BUFFER_STORE_DWORD_OFFSET killed $vgpr0, $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0, 0, 0, 0, implicit $exec
+    S_ENDPGM
+...