[PATCH] D78318: [GlobalISel][InlineAsm] Add support for basic output operand constraints

Mon Apr 27 02:38:28 PDT 2020

kschwarz added inline comments.

================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:693-694
       // regclasses.
-      if (isTargetSpecificOpcode(MI.getOpcode()) && !MI.isPreISelOpcode())
+      if ((isTargetSpecificOpcode(MI.getOpcode()) && !MI.isPreISelOpcode()) ||
+          MI.isInlineAsm())
         continue;
----------------
arsenm wrote:
> kschwarz wrote:
> > kschwarz wrote:
> > > arsenm wrote:
> > > > This part isn't tested, I would leave this for a separate patch. I'm not sure checking for inline asm explicitly is the right thing here
> > > Sure I can remove this change from the patch. I temporarily removed the -stop-after argument from the irtranslator test to run down to assembly, which resulted in a crash in the register bank selector of AArch64.
> > > Since we cannot have generic virtual registers on INLINEASM instructions, I figured not trying to select a register bank for these instructions would be fine.
> > After removing this line, another AArch64 test starts failing (stack_guard_remat.ll). It triggers the very same crash in the register bank selector of AArch64: llvm::LLT::getScalarSizeInBits() const: Assertion 'RawData != 0 && "Invalid Type"'.
> > 
> > What do you think would be an appropriate check during register bank selection?
> OK, can you add an explicit regbankselect for asm test then?
> 
I've added a test and extracted the MI.isInlineAsm() check similar to the MI.isDebugInstr() below.

Note, however, that I had to specifically add the register class annotations to the input MIR (for the virtual register definitions of inline asm instructions), because it is not printed by the MIRPrinter by default. This makes the current output of MIRPrinter for inline asm instructions not round-trippable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78318