[PATCH] D13070: [SimplifyCFG] Speculatively flatten CFG based on profiling metadata

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Minor nits inline.  With those addressed, this LGTM (I'd still wait for @hfinkel to take a look).


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2354
@@ +2353,3 @@
+  // values. 
+  Value *Simplified = SimplifyICmpInst(ICmpInst::ICMP_ULE, A, B, DL);
+  if (!Simplified) return false;
----------------
Use `dyn_cast_or_null` to directly cast to `Constant`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2385
@@ +2384,3 @@
+static bool SpeculativelyFlattenCondBranchToCondBranch(BranchInst *PBI, BranchInst *BI,
+                                                            const DataLayout &DL) {
+  auto *PredBB = PBI->getParent();
----------------
Nit: spacing is a little off here.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2405
@@ +2404,3 @@
+  // transformation to that done by SpeculativelyExecuteBB.  We should consider
+  // combining them at some point.
+
----------------
SGTM. :)

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2412
@@ +2411,3 @@
+    if (isa<TerminatorInst>(I)) break;
+    if (!isSafeToSpeculativelyExecute(&I, PBI))
+      return false;
----------------
Should `CtxI` be `nullptr` here?  We're speculating it above `PBI`, so I think we cannot assume control dependence on `PBI`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2417
@@ +2416,3 @@
+    if (SpeculationCost > SpeculativeFlattenThreshold ||
+        isa<CallInst>(I) || isa<InvokeInst>(I))
+      return false;
----------------
Minor nit: `InvokeInst` should have been caught with `isa<TerminatorInst>`.  In fact, we can pretty much `assert(!isa<InvokeInst>(I))` since the only terminator allowed in `BB` is `BI`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2424
@@ +2423,3 @@
+        << *BI->getCondition() << "\n");
+  Value *WhichCond = PBI->getCondition();
+  auto *Success = BI->getSuccessor(0);
----------------
[This is optional to fix, since the actual rewriting is fairly simple]

I'd put a small "before" and "after" IR snippet here, using the C++ identifiers in IR, like:

```
// Before:
//
//  br %whichCond, label %bb, label %fail1  ;; == PBB
//
// bb:
//  br %cond, label %success, label %fail2  ;; == BB
//
```
etc.

I think that will make the graph rewriting clearer.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2427
@@ +2426,3 @@
+  auto *Fail1 = PBI->getSuccessor(1);
+  auto *Fail2 =  BI->getSuccessor(1);
+  // Have PBI branch directly to the fast path using BI's condition, branch
----------------
I'd call these `FailBI` and `FailPBI`.


http://reviews.llvm.org/D13070