[PATCH] D53765: [RFC prototype] Implementation of asm-goto support in LLVM

[Craig Topper via Phabricator via llvm-commits]

craig.topper marked 3 inline comments as done and an inline comment as not done.
craig.topper added inline comments.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4277-4279
+      if (!FTy->isVarArg()) {
+        if (OpNum != Record.size())
+          return error("Invalid record");
----------------
nickdesaulniers wrote:
> ```
> if (x)
>   if (y)
>     foo();
> ```
> ->
> ```
> if (x && y)
>   foo();
> ```
That would change the meaning of the else on the first if wouldn't it?


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3001
+  // terminator and won't pop up on the previous checks. This checks whether
+  // the last non-terminator instruction is an inline asm referencing this BB.
+  // If it is, we assume asm-goto with a jump to here and a fallthrough that got
----------------
nickdesaulniers wrote:
> Is this comment about `terminator instruction` still relevant?
I think so. The CodeGen layer INLINEASM is not a terminator instruction.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3007
+    // FIXME: this is crude, but the asm-goto sometimes seems to be the second-
+    // to-last operand. Find a prettier way to check this.
+    const auto &AG = *AGI;
----------------
nickdesaulniers wrote:
> This comment is also added to lib/CodeGen/MachineBasicBlock.cpp. What does that mean for "asm-goto sometimes seems to be the second-to-last operand?" Second to last operand to what?
I think this meant "instruction" not "operand"


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

https://reviews.llvm.org/D53765