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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:105
+  if (!RC)
+    return;
+
----------------
This should probably propagate that this is an error with the function return value?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:120
+
+  // If this associated to a specific register, initialize iterator to correct
+  // place. If virtual, make sure we have enough registers
----------------
Specify physical register in comment?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:128-131
+  if (AssignedReg) {
+    for (; *I != AssignedReg; ++I)
+      assert(RC->contains(*I) && "AssignedReg should be member of RC");
+  }
----------------
Looking again this assert doesn't really make sense. You are finding the registers from the reg class iterator, so obviously it is in the class already


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:133
+
+  for (; NumRegs; --NumRegs, ++I) {
+    assert(I != RC->end() && "Ran out of registers to allocate!");
----------------
Picking multiple physical registers by scanning through the register class doesn't make any sense to me. I would expect NumRegs > 1 only with a virtual register


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:154
+  }
+  llvm_unreachable("Invalid constraint type");
+}
----------------
Move this into default case?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:379
+                      RegState::Define | RegState::EarlyClobber |
+                          getImplRegState(Register::isPhysicalRegister(Reg)));
+        }
----------------
Reg.isPhysical()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78318