GlobalISel: Fix RegBankSelect for REG_SEQUENCE

[arsenm]

Summary

The AArch64 test was broken since the result register already had a
set register class, so this test was a no-op. The mapping verify call
would fail because the result size is not the same as the inputs like
in a copy or phi.

The AMDGPU testcases are half broken and introduce illegal VGPR->SGPR
copies which need much more work to handle correctly (same for phis),
but add them as a baseline.

[qcolombet]

Nice catch!

Comment below.

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp

223

Is it possible that OperandsMapping[0] != nullptr at that point?

Essentially when we set OperandsMapping[0] we have either IsCopyLike == true and we will break out right after we set it or we set it outside of IsCopyLike == true then we will never get into that block.

[arsenm]

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp

223

It's initialized to null by the SmallVector constructor, so this is the first time it's ever set. The IsCopyLike check is a loop invariant.

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually. I'm able to hack around reg_sequence in the backend now, but phi will require changing this and the iteration order to defer mapping phis until the operands are all already mapped

[qcolombet]

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.

Could you elaborate on that?

Essentially the logic here is: if at least one argument is set, we can use that reg bank for the destination, since this is just a glorified copy. I can see how this doesn't work for reg_sequence, but I don't get why phis are a problem.

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp

223

Ok, where I was getting at is, this check is not necessary, since by construction OperandsMapping[0] is nullptr and after that we won't loop back here.

[arsenm]

In D58758#1430195, @qcolombet wrote:

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.

Could you elaborate on that?

Essentially the logic here is: if at least one argument is set, we can use that reg bank for the destination, since this is just a glorified copy. I can see how this doesn't work for reg_sequence, but I don't get why phis are a problem.

I was going to write a longer email when I got back to looking at seriously fixing this, but it's not copy-like when you can't copy from one of the source banks to the destination. If any input is a VGPR, the result must be a VGPR. Phi is complicated because the input banks might not be known yet, so you have to defer assigning it until they are known. You have to allow a partially mapped function, and keep trying to regbankselect until all of the phi operands are resolved. This is essentially what SIFixSGPRCopies has to do now.

[arsenm]

Remove null check. I think this switches to picking the last operand's bank rather than the first, but no tests seem to care

[arsenm]

ping

[arsenm]

r356713

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp

if (!CurRegBank) {
         CompleteMapping = false;
 
         if (!IsCopyLike)
           // MI does not carry enough information to guess the mapping.
           return getInvalidInstructionMapping();
         continue;
       }
     }
-    const ValueMapping *ValMapping =
-        &getValueMapping(0, getSizeInBits(Reg, MRI, TRI), *CurRegBank);
+
+    unsigned Size = getSizeInBits(Reg, MRI, TRI);
+    const ValueMapping *ValMapping = &getValueMapping(0, Size, *CurRegBank);
     if (IsCopyLike) {
+      if (MI.isRegSequence()) {
+        // For reg_sequence, the result size does not match the input.
+        unsigned ResultSize = getSizeInBits(MI.getOperand(0).getReg(),
+                                            MRI, TRI);
+        OperandsMapping[0] = &getValueMapping(0, ResultSize, *CurRegBank);
+      } else {
         OperandsMapping[0] = ValMapping;
+      }
+
       CompleteMapping = true;
       break;
     }
     OperandsMapping[OpIdx] = ValMapping;
   }
 
   if (IsCopyLike && !CompleteMapping)
     // No way to deduce the type from what we have.
     return getInvalidInstructionMapping();

test/CodeGen/AArch64/GlobalISel/regbankselect-reg_sequence.mir

 # RUN: llc %s -mtriple aarch64-- -o - -run-pass regbankselect | FileCheck %s
---- |
-  define void @foo() { ret void }
-...
 ---
 # CHECK-LABEL: foo
 # Check that we produce a valid mapping for REG_SEQUENCE.
 # This used to fail the RegisterBankInfo verify because
 # we were using the exclusively the type of the definition
 # whereas since REG_SEQUENCE are kind of target opcode
 # their definition may not have a type.
 #
-# CHECK: id: 0, class: dd
+
+# CHECK: %0:fpr(s128) = REG_SEQUENCE $d0, %subreg.dsub0, $d1, %subreg.dsub1
+
 name: foo
 legalized: true
 tracksRegLiveness: true
-registers:
-  - { id: 0, class: dd }
 body: |
   bb.0:
     liveins: $d0, $d1
 
-    %0 = REG_SEQUENCE $d0, %subreg.dsub0, $d1, %subreg.dsub1
+    %0:_(s128) = REG_SEQUENCE $d0, %subreg.dsub0, $d1, %subreg.dsub1
 
 ...

test/CodeGen/AMDGPU/GlobalISel/regbankselect-reg-sequence.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=amdgcn -mcpu=fiji -run-pass=regbankselect %s -verify-machineinstrs -o - -regbankselect-fast | FileCheck %s
+# RUN: llc -march=amdgcn -mcpu=fiji -run-pass=regbankselect %s -verify-machineinstrs -o - -regbankselect-greedy | FileCheck %s
+
+---
+name: reg_sequence_ss_vreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $sgpr0, $sgpr1
+
+    ; CHECK-LABEL: name: reg_sequence_ss_vreg
+    ; CHECK: liveins: $sgpr0, $sgpr1
+    ; CHECK: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
+    ; CHECK: [[COPY1:%[0-9]+]]:sgpr(s32) = COPY $sgpr1
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:sgpr(s64) = REG_SEQUENCE [[COPY]](s32), %subreg.sub0, [[COPY1]](s32), %subreg.sub1
+    %0:_(s32) = COPY $sgpr0
+    %1:_(s32) = COPY $sgpr1
+    %2:_(s64) = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+...
+
+---
+name: reg_sequence_ss_physreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $sgpr0, $sgpr1
+
+    ; CHECK-LABEL: name: reg_sequence_ss_physreg
+    ; CHECK: liveins: $sgpr0, $sgpr1
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:sgpr(s64) = REG_SEQUENCE $sgpr0, %subreg.sub0, $sgpr1, %subreg.sub1
+    %0:_(s64) = REG_SEQUENCE $sgpr0, %subreg.sub0, $sgpr1, %subreg.sub1
+...
+
+---
+name: reg_sequence_sv_vreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $sgpr0, $vgpr0
+
+    ; CHECK-LABEL: name: reg_sequence_sv_vreg
+    ; CHECK: liveins: $sgpr0, $vgpr0
+    ; CHECK: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
+    ; CHECK: [[COPY1:%[0-9]+]]:vgpr(s32) = COPY $vgpr0
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:sgpr(s64) = REG_SEQUENCE [[COPY]](s32), %subreg.sub0, [[COPY1]](s32), %subreg.sub1
+    %0:_(s32) = COPY $sgpr0
+    %1:_(s32) = COPY $vgpr0
+    %2:_(s64) = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+...
+
+---
+name: reg_sequence_sv_physreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $sgpr0, $vgpr0
+
+    ; CHECK-LABEL: name: reg_sequence_sv_physreg
+    ; CHECK: liveins: $sgpr0, $vgpr0
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:sgpr(s64) = REG_SEQUENCE $sgpr0, %subreg.sub0, $vgpr0, %subreg.sub1
+    %0:_(s64) = REG_SEQUENCE $sgpr0, %subreg.sub0, $vgpr0, %subreg.sub1
+...
+
+---
+name: reg_sequence_vs_vreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $vgpr0, $sgpr0
+
+    ; CHECK-LABEL: name: reg_sequence_vs_vreg
+    ; CHECK: liveins: $vgpr0, $sgpr0
+    ; CHECK: [[COPY:%[0-9]+]]:vgpr(s32) = COPY $vgpr0
+    ; CHECK: [[COPY1:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:vgpr(s64) = REG_SEQUENCE [[COPY]](s32), %subreg.sub0, [[COPY1]](s32), %subreg.sub1
+    %0:_(s32) = COPY $vgpr0
+    %1:_(s32) = COPY $sgpr0
+    %2:_(s64) = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+...
+
+---
+name: reg_sequence_vs_physreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $vgpr0, $sgpr0
+
+    ; CHECK-LABEL: name: reg_sequence_vs_physreg
+    ; CHECK: liveins: $vgpr0, $sgpr0
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:vgpr(s64) = REG_SEQUENCE $vgpr0, %subreg.sub0, $sgpr0, %subreg.sub1
+    %0:_(s64) = REG_SEQUENCE $vgpr0, %subreg.sub0, $sgpr0, %subreg.sub1
+...
+
+---
+name: reg_sequence_vv_vreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1
+
+    ; CHECK-LABEL: name: reg_sequence_vv_vreg
+    ; CHECK: liveins: $vgpr0, $vgpr1
+    ; CHECK: [[COPY:%[0-9]+]]:vgpr(s32) = COPY $vgpr0
+    ; CHECK: [[COPY1:%[0-9]+]]:vgpr(s32) = COPY $vgpr1
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:vgpr(s64) = REG_SEQUENCE [[COPY]](s32), %subreg.sub0, [[COPY1]](s32), %subreg.sub1
+    %0:_(s32) = COPY $vgpr0
+    %1:_(s32) = COPY $vgpr1
+    %2:_(s64) = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+...
+
+---
+name: reg_sequence_vv_physreg
+legalized: true
+tracksRegLiveness: true
+
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1
+
+    ; CHECK-LABEL: name: reg_sequence_vv_physreg
+    ; CHECK: liveins: $vgpr0, $vgpr1
+    ; CHECK: [[REG_SEQUENCE:%[0-9]+]]:vgpr(s64) = REG_SEQUENCE $vgpr0, %subreg.sub0, $vgpr1, %subreg.sub1
+    %0:_(s64) = REG_SEQUENCE $vgpr0, %subreg.sub0, $vgpr1, %subreg.sub1
+...
+