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

[Konstantin Schwarz via Phabricator via llvm-commits]

kschwarz marked an inline comment as done.
kschwarz added inline comments.


================
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
----------------
arsenm wrote:
> Specify physical register in comment?
Which comment do you mean?


================
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");
+  }
----------------
arsenm wrote:
> 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
I've mostly copied this function from the SelectionDAG code, but I agree this assertion should never trigger. I couldn't find out from the history why this assertion was added, maybe because the target provides both the physical register and the register class, and they have to match?
I'll remove the assertion in the next round.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:133
+
+  for (; NumRegs; --NumRegs, ++I) {
+    assert(I != RC->end() && "Ran out of registers to allocate!");
----------------
arsenm wrote:
> 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
The LLVM IR reference manual contains the following paragraph:
> There is also an “interesting” feature which deserves a bit of explanation: if a register class constraint allocates a register which is too small for the value type operand provided as input, the input value will be split into multiple registers, and all of them passed to the inline asm.

X86 uses this at least with the 'A' register constraint, but there are other targets, too (e.g. ARM uses it to pass 64-bit values in even-odd register pairs).


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:154
+  }
+  llvm_unreachable("Invalid constraint type");
+}
----------------
arsenm wrote:
> Move this into default case?
At least my Xcode warns about adding a default case for a switch which covers all enumeration values. I think there's also a similar notice in the development guide?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78318