[PATCH] D155087: [CodeGen] Fix assertion failure caused by inline assembly

[Bryan Chan via Phabricator via llvm-commits]

bryanpkc added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:944
+  }
   else {
     assert(!isRegUsedInInstr(LRI->PhysReg, LookAtPhysRegUses) &&
----------------
This line should be concatenated to the previous line, to match LLVM coding style.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:953
   MCPhysReg PhysReg = LRI->PhysReg;
   assert(PhysReg != 0 && "Register not assigned");
   if (LRI->Reloaded || LRI->LiveOut) {
----------------
I guess this `assert` is no longer necessary?


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:838
       MI.emitError("inline assembly requires more registers than available");
+      report_fatal_error("inline assembly requires more registers than available", false);
+    }
----------------
Qi-Hu wrote:
> bryanpkc wrote:
> > bryanpkc wrote:
> > > `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?
> > > `MachineInstr::emitError` falls back to calling `report_fatal_error` if it cannot find a `MCContext`.
> > 
> > I meant `LLVMContext`, not `MCContext`.
> `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`.
OK thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155087/new/

https://reviews.llvm.org/D155087