[PATCH] D69876: Allow output constraints on "asm goto"

Thu Jan 9 11:48:21 PST 2020

jyknight added inline comments.

================
Comment at: clang/lib/AST/Stmt.cpp:646-648
+      // Labels are placed after "InOut" operands. Adjust accordingly.
+      if (IsLabel)
+        N += getNumPlusOperands();
----------------
void wrote:
> jyknight wrote:
> > I'm confused about this part. Why isn't the "N" specified in the assembly string already the correct value for the labels? Is the ordering we use internally and that users use externally not the same? I'm assuming your code here is correct, just I'm not understanding, so probably an improved comment would be nice.
> The LLVM-specific ordering that I saw was something like:
> 
>   `<output constraints>,<input constraints>,<plus constraints>,<optional X>,<other constraints>`
> 
> The `<plus constraints>` is empty when there are no output constraints. This just makes up for this. It's probably better to reverse the `<optional X>` and `<plus constraints>` parts, but I didn't know how pervasive the order's assumption is in the code.
Oh, right -- in llvm IR, the "plus" constraints don't exist -- we specify those as separate-but-linked output and input constraints.

But thinking about this more...I think this code is actually incorrect. It should be translating the numbers based on where it finds the matching argument-number, not on the 'l' character being present. (And it should do so for named-arguments too).

So, yes, I think it'd be better to just swap the order, and move the plus-operand tied-inputs to the end of the arglist, where they don't affect numbering. I don't _think_ reversing the order will cause other issues.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876