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

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2394
@@ +2393,3 @@
+
+  // Can we speculate everything in the given block (except for the terminator
+  // instruction) at the instruction boundary before 'At'?
----------------
sanjoy wrote:
> Why is this needed?  Since you're changing `PBI`'s condition to be `BI`'s condition, and `BI`'s condition implies `PBI`'s condition, I don't see what gets speculatively executed.
The problem is that instructions from BI's block are speculated above PBI.  If you had some code like:
if (i > length) return;
%p = load a[i];
if (i + 1 > length) return;

After the transform, we'd end up with:
%p = load a[i];
if (i + 1 > length) {if (i > length) return; else return;}

We can't necessarily speculate the load above the first condition.  



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2493
@@ -2390,1 +2492,3 @@
 
+  if (ConstantExpr *CE = dyn_cast<ConstantExpr>(BI->getCondition()))
+    if (CE->canTrap())
----------------
sanjoy wrote:
> `auto *` is clearer, since the type is obvious from the `dyn_cast`.
Agreed, but for the record, I just moved this line.


http://reviews.llvm.org/D13070