[PATCH] D20143: New pass: guard widening

[Philip Reames via llvm-commits]

reames requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/Transforms/Scalar/GuardWidening.h:12
@@ +11,3 @@
+// that (optimistically) combines multiple guards into one to have fewer checks
+// at runtime.
+//
----------------
You should expand this comment.  Specifically, clarify exceptions around re-execution and healing.

Actually, never mind.  This is available in the source.

================
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);
----------------
Interface wise, this might be clearer as returning an optional<Value*> or just a Value* which can be null.

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

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:213
@@ +212,3 @@
+    auto *CurLoop = LI.getLoopFor(CurBB);
+    const auto &GuardsInCurBB = GuardsInBlock.find(CurBB)->second;
+
----------------
Please add an assert that curBB has been populated in GuardsInBlock.  Your lazy population scheme is fine, but the code doesn't make it *obvious* that's it correct.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:225
@@ +224,3 @@
+      assert(NewEnd != E && "GuardInst not in its own block?");
+      E = NewEnd;
+    }
----------------
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.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:229
@@ +228,3 @@
+    for (auto *Candidate : make_range(I, E)) {
+      auto Score =
+          computeWideningScore(GuardInst, GuardInstLoop, Candidate, CurLoop);
----------------
As far as I understand it, there is never a case where widening one pair of guards would prevent you from merging that widened guard into another.  You code is structured as if this is a possibility and it results in excessive generality.  

The excess generality here is causing you to implement an O(G^2) algorithm for something which should be a collection of a O(N) algorithms.  This is unacceptable.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:253
@@ +252,3 @@
+  GuardInst->setArgOperand(0, ConstantInt::getTrue(GuardInst->getContext()));
+  EliminatedGuards.push_back(GuardInst);
+  WidenedGuards.insert(BestSoFar);
----------------
In general, you *should* be able to simply delete the second guard here.  The fact you can't, indicates there's a problem with the algorithm you've chosen.

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:287
@@ +286,3 @@
+
+bool GuardWideningImpl::isAvailableAt(Value *V, Instruction *Loc) {
+  auto *Inst = dyn_cast<Instruction>(V);
----------------
This really seems likely to be excessive generality again.  You don't need a generic hoisting algorithm.  

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

================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:358
@@ +357,3 @@
+    Result = BinaryOperator::CreateAnd(Cond0, Cond1, "wide.chk", InsertPt);
+  }
+
----------------
Is there possibly a missing return here?


http://reviews.llvm.org/D20143