[RegAlloc] Fix assertion failure caused by inline assembly

[Qi-Hu]

Summary

When inline assembly requests more registers than available, the MachineInstr::emitError function in the RegAllocFast pass emits an error but doesn't stop the pass. Then the compiler crashes at an assertion error. This commit, mimicking the RegAllocGreedy pass, assigns a random physical register, and therefore avoids the crash. This error has been found for both rust and clang, while it doesn't occur in gcc.

[bryanpkc]

llvm/lib/CodeGen/RegAllocFast.cpp

838

MachineInstr::emitError falls back to calling report_fatal_error if it cannot find a MCContext. Making an additional call to report_fatal_error here doesn't feel right. Why isn't MI.emitError able to stop the compiler before it crashes?

[bryanpkc]

llvm/lib/CodeGen/RegAllocFast.cpp

838

MachineInstr::emitError falls back to calling report_fatal_error if it cannot find a MCContext.

I meant LLVMContext, not MCContext.

[Qi-Hu]

llvm/lib/CodeGen/RegAllocFast.cpp

838

emitError is designed to return and does not stop the pass. In fact, given the same test case, RegAllocGreedy also reports the error but ends up generating bogus assembly, without crashing. I have updated the patch to mimic that behaviour, by copying some existing logic from useVirtReg.

[bryanpkc]

llvm/lib/CodeGen/RegAllocFast.cpp

838

OK thanks.

937–946

This line should be concatenated to the previous line, to match LLVM coding style.

954–955

I guess this assert is no longer necessary?

[bryanpkc]

llvm/lib/CodeGen/RegAllocFast.cpp

941

Why doesn't this return the result of setPhysReg?

You should also add a comment to explain that since there was an error finding a suitable register for LRI, we just pick a register at random and bail out of the function immediately.

[bryanpkc]

LGTM, but I would wait for others to confirm whether this is an acceptable fix.

[bryanpkc]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

4

Suggestions:

1. Add a CHECK line to match the output assembly, e.g. ; CHECK: .size main, .Lfunc_end0-main, to ensure that that the backend does not crash after register allocation.
2. Add a RUN line with llc -O2 to ensure that RegAllocGreedy behaves the same way.

[arsenm]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

4

Could also add -verify-machineinstrs, it may fail already with expensive checks. I was working on fixing verifier errors after regalloc failure but haven't finished it

61

Can probably drop most of these attributes

[Qi-Hu]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

4

That's a good point. However, adding MachineVerifier reports a fatal error for llc -O2: "Bad machine code: Using an undefined physical register".

[arsenm]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

4

Which is another bug which needs to be fixed

[arsenm]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

2

Don't need the REQUIRES, it's implied by the x86 directory

[arsenm]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

4

If it's failing the verifier after this, you should check the verification message. You'll need to handle that to not break EXPENSIVE_CHECKS testing. You can fix the additional verifier error separately though

[Qi-Hu]

llvm/test/CodeGen/X86/inline-asm-assertion.ll

4

I don't know how to fix the verifier error at this point, but I agree that it should be handled as a separate issue.

[arsenm]

llvm/lib/CodeGen/RegAllocFast.cpp

941

Should also guard against AllocationOrder being empty which can happen if all the registers become reserved, I guess you just have to use noreg at that point

[bryanpkc]

llvm/lib/CodeGen/RegAllocFast.cpp

941

@arsenm This pass uses emitError to indicate that the IR is no longer valid, but is still required to continue and complete register allocation. If we enter this code path, we don't expect to generate correct code anyway, so I think it should be okay to skip the check, both here and in useVirtReg.

[arsenm]

llvm/lib/CodeGen/RegAllocFast.cpp

941

If AllocationOrder is empty the compiler will just immediately crash which is not helpful

[bryanpkc]

llvm/lib/CodeGen/RegAllocFast.cpp

941

I see, I had misread your comment. Thanks.

[bryanpkc]

Landed as rGddd7d35c6c2697eec8d665e70d4e1395003cbec9.

llvm/lib/CodeGen/RegAllocFast.cpp

for (MCPhysReg PhysReg : AllocationOrder) {
     }
   }
 
   if (!BestReg) {
     // Nothing we can do: Report an error and keep going with an invalid
     // allocation.
     if (MI.isInlineAsm())
       MI.emitError("inline assembly requires more registers than available");
     else
       MI.emitError("ran out of registers during register allocation");
 
     LR.Error = true;
     LR.PhysReg = 0;
     return;
   }
 
   displacePhysReg(MI, BestReg);
if (!MO.isDead()) {
       if (mayLiveOut(VirtReg)) {
         LRI->LiveOut = true;
       } else {
         // It is a dead def without the dead flag; add the flag now.
         MO.setIsDead(true);
       }
     }
   }
-  if (LRI->PhysReg == 0)
+  if (LRI->PhysReg == 0) {
     allocVirtReg(MI, *LRI, 0, LookAtPhysRegUses);
-  else {
+    // If no physical register is available for LRI, we assign one at random
+    // and bail out of this function immediately.
+    if (LRI->Error) {
+      const TargetRegisterClass &RC = *MRI->getRegClass(VirtReg);
+      ArrayRef<MCPhysReg> AllocationOrder = RegClassInfo.getOrder(&RC);
+      if (AllocationOrder.empty())
+        return setPhysReg(MI, MO, MCRegister::NoRegister);
+      return setPhysReg(MI, MO, *AllocationOrder.begin());
+    }
+  } else {
     assert(!isRegUsedInInstr(LRI->PhysReg, LookAtPhysRegUses) &&
            "TODO: preassign mismatch");
     LLVM_DEBUG(dbgs() << "In def of " << printReg(VirtReg, TRI)
                       << " use existing assignment to "
                       << printReg(LRI->PhysReg, TRI) << '\n');
   }
 
   MCPhysReg PhysReg = LRI->PhysReg;
-  assert(PhysReg != 0 && "Register not assigned");
   if (LRI->Reloaded || LRI->LiveOut) {
     if (!MI.isImplicitDef()) {
       MachineBasicBlock::iterator SpillBefore =
           std::next((MachineBasicBlock::iterator)MI.getIterator());
       LLVM_DEBUG(dbgs() << "Spill Reason: LO: " << LRI->LiveOut << " RL: "
                         << LRI->Reloaded << '\n');
       bool Kill = LRI->LastUse == nullptr;
       spill(SpillBefore, VirtReg, PhysReg, Kill, LRI->LiveOut);
 
if (MI.isCopy() && MI.getOperand(1).getSubReg() == 0) {
         assert(Hint.isPhysical() &&
                "Copy destination should already be assigned");
       }
     }
     allocVirtReg(MI, *LRI, Hint, false);
     if (LRI->Error) {
       const TargetRegisterClass &RC = *MRI->getRegClass(VirtReg);
       ArrayRef<MCPhysReg> AllocationOrder = RegClassInfo.getOrder(&RC);
+      if (AllocationOrder.empty())
+        return setPhysReg(MI, MO, MCRegister::NoRegister);
       return setPhysReg(MI, MO, *AllocationOrder.begin());
     }
   }
 
   LRI->LastUse = &MI;
 
   if (MI.getOpcode() == TargetOpcode::BUNDLE) {
     BundleVirtRegsMap[VirtReg] = LRI->PhysReg;

llvm/test/CodeGen/X86/inline-asm-assertion.ll

+; RUN: not llc -verify-machineinstrs -O0 < %s 2>&1 | FileCheck %s
+; RUN: not --crash llc -verify-machineinstrs -O2 < %s 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+; CHECK: error: inline assembly requires more registers than available
+; CHECK: .size   main, .Lfunc_end0-main
+; CHECK-O2: error: inline assembly requires more registers than available
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind optnone uwtable
+define dso_local i32 @main() #0 {
+entry:
+  %r0 = alloca i32, align 4
+  %r1 = alloca i32, align 4
+  %r2 = alloca i32, align 4
+  %r3 = alloca i32, align 4
+  %r4 = alloca i32, align 4
+  %r5 = alloca i32, align 4
+  %r6 = alloca i32, align 4
+  %r7 = alloca i32, align 4
+  %r8 = alloca i32, align 4
+  %r9 = alloca i32, align 4
+  %r10 = alloca i32, align 4
+  %r11 = alloca i32, align 4
+  %r12 = alloca i32, align 4
+  %r13 = alloca i32, align 4
+  %r14 = alloca i32, align 4
+  %0 = call { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } asm "movl $$0, $0;movl $$1, $1;movl $$2, $2;movl $$3, $3;movl $$4, $4;movl $$5, $5;movl $$6, $6;movl $$7, $7;movl $$8, $8;movl $$9, $9;movl $$10, $10;movl $$11, $11;movl $$12, $12;movl $$13, $13;movl $$14, $14;", "=r,=r,=r,=r,=r,=r,=r,=r,=r,=r,=r,=r,=r,=r,=r,~{dirflag},~{fpsr},~{flags}"() #1
+  %asmresult = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 0
+  %asmresult1 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 1
+  %asmresult2 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 2
+  %asmresult3 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 3
+  %asmresult4 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 4
+  %asmresult5 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 5
+  %asmresult6 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 6
+  %asmresult7 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 7
+  %asmresult8 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 8
+  %asmresult9 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 9
+  %asmresult10 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 10
+  %asmresult11 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 11
+  %asmresult12 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 12
+  %asmresult13 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 13
+  %asmresult14 = extractvalue { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32 } %0, 14
+  store i32 %asmresult, ptr %r0, align 4
+  store i32 %asmresult1, ptr %r1, align 4
+  store i32 %asmresult2, ptr %r2, align 4
+  store i32 %asmresult3, ptr %r3, align 4
+  store i32 %asmresult4, ptr %r4, align 4
+  store i32 %asmresult5, ptr %r5, align 4
+  store i32 %asmresult6, ptr %r6, align 4
+  store i32 %asmresult7, ptr %r7, align 4
+  store i32 %asmresult8, ptr %r8, align 4
+  store i32 %asmresult9, ptr %r9, align 4
+  store i32 %asmresult10, ptr %r10, align 4
+  store i32 %asmresult11, ptr %r11, align 4
+  store i32 %asmresult12, ptr %r12, align 4
+  store i32 %asmresult13, ptr %r13, align 4
+  store i32 %asmresult14, ptr %r14, align 4
+  ret i32 0
+}
+
+attributes #0 = { "frame-pointer"="all" }