[PATCH] D56075: [GuardWidening] Support widening of explicitly expressed guards

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:87
+static cl::opt<bool>
+    WidenExplicitGuards("guard-widening-widen-explicit-guards", cl::Hidden,
+                        cl::desc("Whether or not we should widen guards "
----------------
As noted elsewhere, I dislike the explicit terminology.  I'd suggest calling this guard-widening-include-widenable-conditions.

That naming does imply something slightly more general though, see further comments.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:101
   }
+  if (isExplicitGuard(I)) {
+    auto *Cond = cast<BranchInst>(I)->getCondition();
----------------
I really don't like the coupling between the single and matcher and the caller.  What if we later extend the matcher to (v1 and (v2 and WC))?


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:402
+    if (i == (e - 1) && (CurBB->getTerminator() != GuardInst ||
+                         isExplicitGuard(CurBB->getTerminator()))) {
       // Corner case: make sure we're only looking at guards strictly dominating
----------------
Not sure this check is needed?


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:484
     auto *DominatedBlock = DominatedGuard->getParent();
+    if (isExplicitGuard(DominatingGuard))
+      DominatingBlock = cast<BranchInst>(DominatingGuard)->getSuccessor(0);
----------------
Having this adjust only one of the two variables looks awfully suspicious?  


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:581
           Result = new ICmpInst(InsertPt, Pred, LHS, NewRHS, "wide.chk");
+          if (WidenableCondition)
+            Result = BinaryOperator::CreateAnd(Result, WidenableCondition,
----------------
No, just no.  You've screwed up your abstraction here.  WideanbleCondition had better be either Cond1 or Cond2 for this function to have reasonable semantics.  


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

https://reviews.llvm.org/D56075