RA: Remove assert on empty live intervals

[arsenm]

Summary

This is possible if there is an undef use when
splitting the vreg during spilling.

Fixes bug 33620.

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

If that's empty and legit, I believe we shouldn't try to enqueue it.

[arsenm]

lib/CodeGen/RegAllocBase.cpp

147

It still needs to be allocated to a register, otherwise an undef vreg use is left around

[MatzeB]

LGTM.
The following code is obviously able to handle it (as we can have the same situation from non-split intervals where no assert exists). So I guess this was a sanity check on the spiller/splitter, let's hope we are fine without it.

[arsenm]

r308808

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

The problem IIRC is that I believe later code is not fine with that.
(BTW, you could have waited for the discussion to be over before committing...)

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

Regardless of the surrounding code working or not in that case, having an empty live-range that needs a register sounds plain wrong to me.

[arsenm]

lib/CodeGen/RegAllocBase.cpp

147

Sorry, I had talked to Matthias about this earlier. The assert is also fairly new and was added last year in r279625 with subregister changes.

[MatzeB]

lib/CodeGen/RegAllocBase.cpp

147

An empty live range that needs a register looks like this and is valid IMO and the normal code handled it fine in my experiments (but it needs to assign a register obviously):

= USE vreg0<undef>

[MatzeB]

lib/CodeGen/RegAllocBase.cpp

147

Ah the infamous r279625, let's add Krzysztof to the reviwers.

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

I am fine with removing the assert. I'm against enqueueing an empty live-range though. More specifically, I am fine with enqueueing it as long as it is valid not to assign it a register.
We have code that makes that assumption (e.g., in last chance recoloring).

[MatzeB]

lib/CodeGen/RegAllocBase.cpp

147

We need to enqueu it so it gets a register assigned. I think those empty liveranges will always succedd when we try to assign a register (because they interfer with nothing) so we should never hit the recolring, splitting etc. phases for them.

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

= USE vreg0<undef>

That's different.
That says we don't care about the value of vreg0 here, not that its live-range is empty.

What I am saying is that I would rather that we come up with a different concept than overloading empty here, because it weakens our ability to find bugs.

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

We need to enqueu it so it gets a register assigned.

Yes because how we conflates the semantic of empty and need a register. But I don't think that's right.

[...] so we should never hit the recolring[...]

Although that's probably true, each time I see should, I think potential bugs :P

[MatzeB]

lib/CodeGen/RegAllocBase.cpp

147

undef operand by construction have no influence on the liverange. They are perfectly valid operands that use vregs, but they have absolutely no effect on the liverange. Hence we end up with an empty liverange. That is just how things work AFAIK.

[qcolombet]

lib/CodeGen/RegAllocBase.cpp

147

undef operand by construction have no influence on the liverange. They are perfectly valid operands that use vregs, but they have absolutely no effect on the liverange. Hence we end up with an empty liverange.
That is just how things work AFAIK.

I agree that empty live-range model the right thing.
What I am saying is that I want to distinguish between something that does not affect liveness because we don't care about its value (this we should enqueue) and something that does not affect liveness but should be here (e.g., a bug that produced and invalid live-range).

To be concrete, I think we want to do instead:
assert((!LI->empty() || !MRI.reg_nodbg_empty()) && "Empty and not used live-range?!")

lib/CodeGen/RegAllocBase.cpp

for (VirtRegVec::iterator I = SplitVRegs.begin(), E = SplitVRegs.end();
       assert(!VRM->hasPhys(SplitVirtReg->reg) && "Register already assigned");
       if (MRI->reg_nodbg_empty(SplitVirtReg->reg)) {
         DEBUG(dbgs() << "not queueing unused  " << *SplitVirtReg << '\n');
         aboutToRemoveInterval(*SplitVirtReg);
         LIS->removeInterval(SplitVirtReg->reg);
         continue;
       }
       DEBUG(dbgs() << "queuing new interval: " << *SplitVirtReg << "\n");
-      assert(!SplitVirtReg->empty() && "expecting non-empty interval");
       assert(TargetRegisterInfo::isVirtualRegister(SplitVirtReg->reg) &&
              "expect split value in virtual register");
       enqueue(SplitVirtReg);
       ++NumNewQueued;
     }
   }
 }
 
 void RegAllocBase::postOptimization() {
   spiller().postOptimization();
   for (auto DeadInst : DeadRemats) {
     LIS->RemoveMachineInstrFromMaps(*DeadInst);
     DeadInst->eraseFromParent();
   }
   DeadRemats.clear();
 }

test/CodeGen/AMDGPU/spill-empty-live-interval.mir

+# RUN: llc -mtriple=amdgcn-amd-amdhsa-opencl -verify-machineinstrs -stress-regalloc=1 -start-before=simple-register-coalescing -stop-after=greedy -o - %s | FileCheck %s
+# https://bugs.llvm.org/show_bug.cgi?id=33620
+
+--- |
+
+  define amdgpu_kernel void @expecting_non_empty_interval() #0 {
+    ret void
+  }
+
+  attributes #0 = { nounwind }
+
+...
+---
+# This would assert due to the empty live interval created for %vreg9
+# on the last S_NOP with an undef subreg use.
+
+# CHECK-LABEL: name: expecting_non_empty_interval
+
+# CHECK: undef %7.sub1 = V_MAC_F32_e32 0, undef %1, undef %7.sub1, implicit %exec
+# CHECK-NEXT: SI_SPILL_V64_SAVE %7, %stack.0, %private_rsrc_reg, %fp_reg, 0, implicit %exec :: (store 8 into %stack.0, align 4)
+# CHECK-NEXT: undef %5.sub1 = V_MOV_B32_e32 1786773504, implicit %exec
+# CHECK-NEXT: dead %2 = V_MUL_F32_e32 0, %5.sub1, implicit %exec
+
+# CHECK: S_NOP 0, implicit %6.sub1
+# CHECK-NEXT: %8 = SI_SPILL_V64_RESTORE %stack.0, %private_rsrc_reg, %fp_reg, 0, implicit %exec :: (load 8 from %stack.0, align 4)
+# CHECK-NEXT: S_NOP 0, implicit %8.sub1
+# CHECK-NEXT: S_NOP 0, implicit undef %9.sub0
+
+name: expecting_non_empty_interval
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: vreg_64, preferred-register: '' }
+  - { id: 1, class: vgpr_32, preferred-register: '' }
+  - { id: 2, class: vgpr_32, preferred-register: '' }
+  - { id: 3, class: vreg_64, preferred-register: '' }
+body:             |
+  bb.0:
+    successors: %bb.1
+    undef %0.sub1 = V_MAC_F32_e32 0, undef %1, undef %0.sub1, implicit %exec
+    undef %3.sub1 = V_MOV_B32_e32 1786773504, implicit %exec
+    dead %2 = V_MUL_F32_e32 0, %3.sub1, implicit %exec
+
+  bb.1:
+    S_NOP 0, implicit %3.sub1
+    S_NOP 0, implicit %0.sub1
+    S_NOP 0, implicit undef %0.sub0
+    S_ENDPGM
+
+...