[PATCH] D20143: New pass: guard widening

[Sanjoy Das via llvm-commits]

sanjoy marked an inline comment as done.
sanjoy added a comment.

The non- O(n^2) bits have been addressed.  As discussed above, there still is pending work to make this scale well with production ready workloads (I've noted this in the pass itself).


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:120
@@ +119,3 @@
+  /// InsertPt and return it in \p Result (else no change to the IR is made).
+  bool widenCondCommon(Value *Cond0, Value *Cond1, Instruction *InsertPt,
+                       Value *&Result);
----------------
reames wrote:
> Interface wise, this might be clearer as returning an optional<Value*> or just a Value* which can be null.
That won't work (the return value does not indicate if `Result` was set) -- there really are two independent results.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:126
@@ +125,3 @@
+  bool isWideningCondProfitable(Value *Cond0, Value *Cond1) {
+    Value *ResultUnused;
+    return widenCondCommon(Cond0, Cond1, /*InsertPt=*/nullptr, ResultUnused);
----------------
reames wrote:
> Generating extra IR just for checking profitability seems less than idea.  Could this be combined into a single function which does the replacement if profitable and nothing otherwise?
It doesn't generate extra IR if `InsertPt` is `nullptr`.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:225
@@ +224,3 @@
+      assert(NewEnd != E && "GuardInst not in its own block?");
+      E = NewEnd;
+    }
----------------
reames wrote:
> You have an assumption here that the guards are recorded in the order they appeared in the original basic block.  Please document that near the definition of the data structure.
That was obvious at the construction site, so I've added an assert here.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:300
@@ +299,3 @@
+
+  return all_of(Inst->operands(), [&](Value *Op) {
+      return isAvailableAt(Op, Loc);
----------------
reames wrote:
> Buggy in the face of unreachable code:
> %a = add i32 %a, 1
Added an assert.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:358
@@ +357,3 @@
+    Result = BinaryOperator::CreateAnd(Cond0, Cond1, "wide.chk", InsertPt);
+  }
+
----------------
reames wrote:
> Is there possibly a missing return here?
No, we're falling through to the `return false;` below.


http://reviews.llvm.org/D20143