[PATCH] D87298: [IRSim][IROutliner] Merging output blocks for extracted functions with outputs

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:827
+/// It is possible that there is a basic block that already performs the same
+/// stores. If there is, we remove it the new output block.  If it does not,
+/// we add it to our list of output blocks.
----------------
s/it the new/it from the new/ ?

Also, since this function is just doing that analysis and not actually doing the removal/addition, the "action" part of this sentence probably belongs near the call site instead.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:851
+    for (Instruction &I : *CompBB) {
+      if (BranchInst *BI = dyn_cast<BranchInst>(&I))
+        continue;
----------------
`isa<BranchInst>(I)`, since `BI` is unused.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:956
+                    << Region.ExtractedFunction << " to "
+                    << MatchingBB.getValue());
+  if (MatchingBB.hasValue()) {
----------------
This call to `getValue()` comes before the check for `hasValue()`. Maybe you want this instead:

```
if (Optional<unsigned> MatchingBB =
      findDuplicateOutputBlock(OutputBB, OutputStoreBBs)) {
  ...
```

and then sink the debug prints into it.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1013
+  // If there needs to be stores, move them from the output block to the end
+  // block to save on branching instructions
+  if (OutputStoreBBs.size() == 1) {
----------------



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

https://reviews.llvm.org/D87298