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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:118-123
+  // If this is a constraint for a specific physical register assign it now.
+
+  // If this associated to a specific register, initialize iterator to correct
+  // place. If virtual, make sure we have enough registers
+
+  // Initialize iterator if necessary
----------------
Comments seem out of place here?

Move them closer to the code that corresponds to them?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:128-131
+  if (AssignedReg) {
+    for (; *I != AssignedReg; ++I)
+      ;
+  }
----------------
Maybe just use `std::find_if`?

Also what should happen if you don't find the assigned register here?



================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:179
+    // load of a register), otherwise we must use 'r'.
+    if ((CType == TargetLowering::C_Other ||
+         CType == TargetLowering::C_Immediate)) {
----------------
Extra parens around `if`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:222-224
+    if (isa<BasicBlock>(v) || isa<ConstantInt>(v) || isa<Function>(v)) {
+      return;
+    }
----------------
Remove braces on single-line if?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:315
+
+        // Add information to the INLINEASM node to know about this output.
+        unsigned OpFlags = InlineAsm::getFlagWord(InlineAsm::Kind_Mem, 1);
----------------
s/node/instruction/


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:354-358
+        for (unsigned j = 0, e = OpInfo.Regs.size(); j < e; j++) {
+          Register Reg = OpInfo.Regs[j];
+          Inst.addReg(Reg,
+                      RegState::Define | getImplRegState(Reg.isPhysical()));
+        }
----------------
Can this be a range-based for?


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -mtriple=aarch64-darwin-ios13 -O0 -global-isel -stop-after=irtranslator -o - %s | FileCheck %s
+
----------------
-verify-machineinstrs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78318