GlobalISel: Disallow undef generic virtual register uses

[arsenm]

Summary

With an undef operand, it's possible for getVRegDef to fail and return
null. This is an edge case very little code bothered to
consider. Proper gMIR should use G_IMPLICIT_DEF instead.

I initially tried to apply this restriction to all SSA MIR, so then
getVRegDef would never fail anywhere. However, ProcessImplicitDefs
does technically run while the function is in SSA. ProcessImplicitDefs
and DetectDeadLanes would need to either move, or a new pseudo-SSA
type of function property would need to be introduced.

[arsenm]

Remove leftover test change

[paquette]

LGTM

[dsanders]

@vsk + @aprantl: I believe this is going to need an exception for DBG_VALUE. Could you confirm if that's correct?

[aprantl]

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

[arsenm]

In D82765#2121181, @aprantl wrote:

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

You can set the undef flag on the MachineOperand.

ProcessImplicitDefs turns

%0 = IMPLICIT_DEF
FOO %0

into

FOO undef %0

You can also hand write this, but I don't think any pass before ProcessImplicitDefs and co should be using these

[aprantl]

From that it *sounds* like a DBG_VALUE of an undef vreg doesn't make sense?

[dsanders]

In D82765#2121196, @arsenm wrote:

In D82765#2121181, @aprantl wrote:

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

You can set the undef flag on the MachineOperand.

ProcessImplicitDefs turns

%0 = IMPLICIT_DEF
FOO %0

into

FOO undef %0

You can also hand write this, but I don't think any pass before ProcessImplicitDefs and co should be using these

Ah ok. This isn't quite what I thought it was when I quickly looked through earlier. The case I was worried about was when DBG_VALUE loses track of a variable and uses $noreg to indicate the register+value is undefined. Whereas this patch is concerned with a known vreg whose contents is undefined and lacks a defining instruction saying that.

[dsanders]

From that it *sounds* like a DBG_VALUE of an undef vreg doesn't make sense?

I suppose even if it can happen then as far as DBG_VALUE is concerned it would be a known location (because it's not so concerned with the actual value as where the value is) and so applying the same rules makes sense.

[arsenm]

llvm/lib/CodeGen/MachineVerifier.cpp

1704

The isSSA check here is a leftover and irrelevant for generic vregs

[arsenm]

e9eab30339a70596386b175b415167cc97e062d5

llvm/lib/CodeGen/MachineVerifier.cpp

if (Register::isPhysicalRegister(Reg)) {
         return;
       }
     } else {
       // Virtual register.
       const TargetRegisterClass *RC = MRI->getRegClassOrNull(Reg);
       if (!RC) {
         // This is a generic virtual register.
 
+        // Do not allow undef uses for generic virtual registers. This ensures
+        // getVRegDef can never fail and return null on a generic register.
+        //
+        // FIXME: This restriction should probably be broadened to all SSA
+        // MIR. However, DetectDeadLanes/ProcessImplicitDefs technically still
+        // run on the SSA function just before phi elimination.
+        if (MO->isUndef() && MRI->isSSA())
+          report("Generic virtual register use cannot be undef", MO, MONum);
+
         // If we're post-Select, we can't have gvregs anymore.
         if (isFunctionSelected) {
           report("Generic virtual register invalid in a Selected function",
                  MO, MONum);
           return;
         }
 
         // The gvreg must have a type and it must not have a SubIdx.

llvm/test/MachineVerifier/generic-vreg-undef-use.mir

+# RUN: not --crash llc -march=aarch64 -o /dev/null -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
+# REQUIRES: aarch64-registered-target
+
+# Undef uses are illegal for generic vregs.
+
+---
+name:            test_undef_use
+liveins:
+body:             |
+  bb.0:
+    %0:_(s32) = G_CONSTANT i32 0
+
+    ; Test generic instruction
+    ; CHECK: *** Bad machine code: Generic virtual register use cannot be undef ***
+    G_STORE %0, undef %1:_(p0) :: (store 4)
+
+    ; Make sure this fails on a post-isel generic instruction.
+    ; CHECK: *** Bad machine code: Generic virtual register use cannot be undef ***
+    $x0 = COPY undef %2:_(s64)
+
+    ; Make sure this fails with a target instruction
+    ; CHECK: *** Bad machine code: Generic virtual register use cannot be undef ***
+    RET_ReallyLR implicit $x0, implicit undef %3:_(s32)
+...
+

llvm/test/MachineVerifier/verifier-phi.mir

 # RUN: llc -o - %s -mtriple=x86_64-- -verify-machineinstrs -run-pass=none | FileCheck %s
 # REQUIRES: x86-registered-target
 
 # This should cleanly pass the machine verifier
 ---
 # CHECK-LABEL: name: func0
-# CHECK: %0:gr32 = PHI undef %1:gr32, %bb.0, undef %1:gr32, %bb.1
+# CHECK: %1:gr32 = PHI %0, %bb.0, %0, %bb.1
 name: func0
 tracksRegLiveness: true
 body: |
   bb.0:
+    %0:gr32 = IMPLICIT_DEF
     JCC_1 %bb.1, 4, implicit undef $eflags
     JMP_1 %bb.2
 
   bb.1:
 
   bb.2:
-    %0 : gr32 = PHI undef %1 : gr32, %bb.0, undef %1 : gr32, %bb.1
+    %2 : gr32 = PHI %0 : gr32, %bb.0, %0 : gr32, %bb.1
 ...
 ---
 # CHECK-LABEL: name: func1
 # CHECK: %2:gr32 = PHI %0, %bb.0, %1, %bb.1
 name: func1
 tracksRegLiveness: true
 body: |
   bb.0: