AMDGPU: Allow additional implicit operands on MOVRELS instructions

[nhaehnle]

Summary

The post-RA scheduler occasionally uses additional implicit operands when
the vector implicit operand as a whole is killed, but some subregisters
are still live because they are directly referenced later. Unfortunately,
this seems incredibly subtle to reproduce.

Fixes piglit spec/glsl-110/execution/variable-indexing/vs-temp-array-mat2-index-wr.shader_test
and others.

[arsenm]

Can you add a test for this?

[nhaehnle]

Adding a test case.

The test case is quite fragile because the problem comes from
ScheduleDAGInstrs::fixupKills, which seems to have a bunch of subtle
problems with sub-registers, and generally the code just doesn't really
do what the comments seem to think it does. But fixing that may not be
worth it since the whole thing has a FIXME on it suggesting to switch it
all to LivePhysRegs.

[• tstellarAMD]

In D25656#572773, @nhaehnle wrote:

Adding a test case.

The test case is quite fragile because the problem comes from
ScheduleDAGInstrs::fixupKills, which seems to have a bunch of subtle
problems with sub-registers, and generally the code just doesn't really
do what the comments seem to think it does. But fixing that may not be
worth it since the whole thing has a FIXME on it suggesting to switch it
all to LivePhysRegs.

Would it be easier to write an MIR tests case?

[nhaehnle]

Use a MIR test instead.

[• tstellarAMD]

LGTM.

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

if (Desc.getOpcode() == AMDGPU::V_MOVRELS_B32_e32 ||
       Desc.getOpcode() == AMDGPU::V_MOVRELD_B32_e64) {
     const bool IsDst = Desc.getOpcode() == AMDGPU::V_MOVRELD_B32_e32 ||
                        Desc.getOpcode() == AMDGPU::V_MOVRELD_B32_e64;
 
     const unsigned StaticNumOps = Desc.getNumOperands() +
       Desc.getNumImplicitUses();
     const unsigned NumImplicitOps = IsDst ? 2 : 1;
 
-    if (MI.getNumOperands() != StaticNumOps + NumImplicitOps) {
+    // Allow additional implicit operands. This allows a fixup done by the post
+    // RA scheduler where the main implicit operand is killed and implicit-defs
+    // are added for sub-registers that remain live after this instruction.
+    if (MI.getNumOperands() < StaticNumOps + NumImplicitOps) {
       ErrInfo = "missing implicit register operands";
       return false;
     }
 
     const MachineOperand *Dst = getNamedOperand(MI, AMDGPU::OpName::vdst);
     if (IsDst) {
       if (!Dst->isUse()) {
         ErrInfo = "v_movreld_b32 vdst should be a use operand";

llvm/trunk/test/CodeGen/MIR/AMDGPU/movrels-bug.mir

+# RUN: llc -march=amdgcn -verify-machineinstrs -run-pass post-RA-sched  %s -o - | FileCheck %s
+
+# This tests a situation where a sub-register of a killed super-register operand
+# of V_MOVRELS happens to have an undef use later on. This leads to the post RA
+# scheduler adding additional implicit operands to the V_MOVRELS, which used
+# to fail machine instruction verification.
+
+--- |
+
+  define amdgpu_vs void @main(i32 %arg) { ret void }
+
+...
+---
+# CHECK-LABEL: name: main
+# CHECK-LABEL: bb.0:
+# CHECK: V_MOVRELS_B32_e32
+# CHECK: V_MAC_F32_e32
+
+name:            main
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    %m0 = S_MOV_B32 undef %sgpr0
+    V_MOVRELD_B32_e32 undef %vgpr2, 0, implicit %m0, implicit %exec, implicit-def %vgpr1_vgpr2_vgpr3_vgpr4_vgpr5_vgpr6_vgpr7_vgpr8, implicit undef %vgpr1_vgpr2_vgpr3_vgpr4_vgpr5_vgpr6_vgpr7_vgpr8(tied-def 4)
+    %m0 = S_MOV_B32 undef %sgpr0
+    %vgpr1 = V_MOVRELS_B32_e32 undef %vgpr1, implicit %m0, implicit %exec, implicit killed %vgpr1_vgpr2_vgpr3_vgpr4_vgpr5_vgpr6_vgpr7_vgpr8
+    %vgpr4 = V_MAC_F32_e32 undef %vgpr0, undef %vgpr0, undef %vgpr4, implicit %exec
+    EXP 15, 12, 0, 1, 0, undef %vgpr0, killed %vgpr1, killed %vgpr4, undef %vgpr0, implicit %exec
+    S_ENDPGM
+
+...