This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][InlineAsm] Fix matching input constraints to mem operand
ClosedPublic

Authored by Petar.Avramovic on Jul 6 2020, 9:00 AM.

Details

Summary

Mark matching input constraint to mem operand as not supported.

Diff Detail

Event Timeline

Petar.Avramovic created this revision.Jul 6 2020, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 9:00 AM

This fixes assert and makes this test mentioned in D82651 to fallback for aarch64.
This seem to have different handling on different targets. I don't know what is the best way to handle this since it looks to me that custom handling has to be done in different places
Just adding same flag and register as MatchedOperand works for this example but from this comment

// We need to make sure that this one operand does not end up in XZR, thus
// require the address to be in a PointerRegClass register.

in AArch64DAGToDAGISel::SelectInlineAsmMemoryOperand an extra copy to a register with PointerRegClass needs to be done. Something like:

--- a/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
@@ -356,7 +356,11 @@ bool InlineAsmLowering::lowerInlineAsm(
         assert(
             SourceRegs.size() == 1 &&
             "Expected the memory output to fit into a single virtual register");
-        Inst.addReg(SourceRegs[0]);
+        const TargetRegisterClass *TRC =
+            MF.getSubtarget().getRegisterInfo()->getPointerRegClass(MF);
+        Register Tmp = MRI->createVirtualRegister(TRC);
+        MIRBuilder.buildCopy(Tmp, SourceRegs[0]);
+        Inst.addReg(Tmp);
       } else {
         // Otherwise, this outputs to a register (directly for C_Register /
         // C_RegisterClass. Find a register that we can use.
@@ -406,6 +410,19 @@ bool InlineAsmLowering::lowerInlineAsm(
           InstFlagIdx += getNumOpRegs(*Inst, InstFlagIdx) + 1;
         assert(getNumOpRegs(*Inst, InstFlagIdx) == 1 && "Wrong flag");
 
+        unsigned MatchedOperandFlag = Inst->getOperand(InstFlagIdx).getImm();
+        if (InlineAsm::isMemKind(MatchedOperandFlag)) {
+          Inst.addImm(MatchedOperandFlag);
+          Inst.addReg(Inst->getOperand(InstFlagIdx + 1).getReg());
+          break;
+        }
+
+        if (!InlineAsm::isRegDefKind(MatchedOperandFlag) &&
+            !InlineAsm::isRegDefEarlyClobberKind(MatchedOperandFlag)) {
+          LLVM_DEBUG(dbgs() << "Unknown matching constraint\n");
+          return false;
+        }
+
         // We want to tie input to register in next operand.
         unsigned DefRegIdx = InstFlagIdx + 1;
         Register Def = Inst->getOperand(DefRegIdx).getReg();

but target should create copy / add immediate and register to Inst.

arsenm added a comment.Jul 6 2020, 1:34 PM

getPointerRegClass is terrible, but also shouldn't be necessary in GlobalISel since pointer types aren't lost

llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
417

I don't think this case is covered in the test?

Petar.Avramovic marked an inline comment as done.Jul 7 2020, 8:13 AM
Petar.Avramovic added inline comments.
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
417

That was covered in D82651 but I didn't check the flag and missed to report that isMemKind() was unsupported .

arsenm accepted this revision.Jul 7 2020, 8:18 AM
This revision is now accepted and ready to land.Jul 7 2020, 8:18 AM
This revision was automatically updated to reflect the committed changes.