void marked an inline comment as done. void added a comment. In D69876#1812724 <https://reviews.llvm.org/D69876#1812724>, @jyknight wrote:
> Reopening, since this didn't actually land yet. BTW, this review still has > the wrong contents in the latest uploaded-diff (showing llvm changes, not > clang changes). *Pinches bridge of nose* *Curses under breath* ================ Comment at: clang/lib/AST/Stmt.cpp:646-648 + // Labels are placed after "InOut" operands. Adjust accordingly. + if (IsLabel) + N += getNumPlusOperands(); ---------------- jyknight wrote: > 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. I agree that moving them would make the most sense. Let me do some experiments to see if it breaks anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69876/new/ https://reviews.llvm.org/D69876 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
