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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the initial work for this patch, and the rebase.



================
Comment at: include/llvm/Analysis/SparsePropagation.h:347
+  }
+
   SwitchInst &SI = cast<SwitchInst>(TI);
----------------
Can this be combined with the previous 2 blocks that have the same bodies?
```
if (x)
  bar();
if (y)
  bar();
if (z)
  bar();
```
->
```
if (x || y || z)
  bar();
```


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4277-4279
+      if (!FTy->isVarArg()) {
+        if (OpNum != Record.size())
+          return error("Invalid record");
----------------
```
if (x)
  if (y)
    foo();
```
->
```
if (x && y)
  foo();
```


================
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
----------------
Is this comment about `terminator instruction` still relevant?


================
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;
----------------
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?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2540-2541
+  // catchswitch for successors.
+  MachineBasicBlock *Return = FuncInfo.MBBMap[I.getSuccessor(0)];
+  const SmallVector<BasicBlock *, 16> Transfers = I.getTransfers();
+
----------------
These can probably be moved closer to their uses below?


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:135
+      // Same for callbr.
+      if (isa<CallBrInst>(P->getTerminator())) return nullptr;
 
----------------
>From this part of the patch through below, there's multiple cases of:
```
if (indirectbr)
  foo();
if (callbr)
  foo();
```
That could be:
```
if (indirectbr || callbr)
  foo();
```


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

https://reviews.llvm.org/D53765