[PATCH] D77535: [GlobalISel] Add extended inline assembler support

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1634
+                                 MachineIRBuilder &MIRBuilder,
+                                 MachineInstrBuilder MIB,
+                                 GISelAsmOperandInfo &OpInfo,
----------------
Not sure why this is here, but it's weird to pass a MachineInstrBuilder to something


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1647
+  // register class, find it.
+  unsigned AssignedReg;
+  const TargetRegisterClass *RC;
----------------
s/unsigned/Register


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1816
 
-  unsigned ExtraInfo = 0;
-  if (IA.hasSideEffects())
-    ExtraInfo |= InlineAsm::Extra_HasSideEffects;
-  if (IA.getDialect() == InlineAsm::AD_Intel)
-    ExtraInfo |= InlineAsm::Extra_AsmDialect;
+      llvm::Type *OpTy = OpInfo.CallOperandVal->getType();
 
----------------
Don't need llvm::


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1912
+        if (!OpInfo.Regs.empty() &&
+            Register::isVirtualRegister(OpInfo.Regs.front())) {
+          // Put the register class of the virtual registers in the flag word.
----------------
I think .isVirtual() should work here


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1946
+            LLVM_DEBUG(dbgs() << "Don't know how to handle tied "
+                                 "indirect register inputs yet");
+            return false;
----------------
Need a newline here


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1971
+                dbgs()
+                << "Couldn't retrieve register class of tied register operand");
+            return false;
----------------
Needs newline


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1973
+            return false;
+            ;
+          }
----------------
Extra semicolon


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1979
+        assert(MatchedOpInfo.ConstraintType == TargetLowering::C_Memory);
+        LLVM_DEBUG(dbgs() << "Tied memory operands not supported yet");
+        return false;
----------------
Missing newline


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1986
+        LLVM_DEBUG(dbgs() << "Indirect input operands with unknown constraint "
+                             "not supported yet");
+        return false;
----------------
Missing newline


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1997
+          LLVM_DEBUG(dbgs() << "Can't handle target constraint "
+                            << OpInfo.ConstraintCode << " yet");
+          return false;
----------------
Missing newline


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2027
+                             "for constraint '"
+                          << OpInfo.ConstraintCode << "'");
+        return false;
----------------
Missing newline


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2035
+            dbgs()
+            << "Couldn't allocate input register for register constraint");
+        return false;
----------------
Missing newline


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2122
+    case TargetLowering::C_Unknown:
+      LLVM_DEBUG(dbgs() << "Unexpected unknown constraint");
+      return false;
----------------
Missing newline


================
Comment at: llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp:860
+      Register SrcReg = Src.getReg();
+      MachineIRBuilder MIB(I);
+      MIB.buildCopy({DstReg}, {SrcReg});
----------------
Construct this once at the start of the function?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp:861
+      MachineIRBuilder MIB(I);
+      MIB.buildCopy({DstReg}, {SrcReg});
+      Src.setReg(DstReg);
----------------
I would expect you just need to constrain the vreg to the class and don't necessarily need a copy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77535