[PATCH] D78319: [GlobalISel][InlineAsm] Add support for basic input operand constraints

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InlineAsmLowering.h:38
 
+  /// Lower the specified operand into the Ops vector.  If it is invalid, don't
+  /// add anything to Ops.
----------------
Doxygen formatting for `Ops`?

```
\p Ops
```

Also can you add Doxygen comments for `Val` and `Constraint`?




================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InlineAsmLowering.h:40
+  /// add anything to Ops.
+  virtual void lowerAsmOperandForConstraint(Value *Val, std::string &Constraint,
+                                            std::vector<MachineOperand> &Ops,
----------------
- Can this return a `bool`? It may be useful to know if this succeeds or fails.
- Can `Constraint` be a `StringRef`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:277
+      if (const auto *BB = dyn_cast<BasicBlock>(OpInfo.CallOperandVal)) {
+        LLVM_DEBUG(dbgs() << "Basic block input operands not supported yet");
+        return false;
----------------
Missing newline in debug string.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:431
+        Inst.add(Ops);
+
+        break;
----------------
Should this newline be here? clang-format?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:481
+      Inst.addImm(Flag);
+      for (unsigned i = 0; i < NumRegs; i++) {
+        MIRBuilder.buildCopy(OpInfo.Regs[i], SourceRegs[0]);
----------------
Can you use a range-based for loop here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78319