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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 3 inline comments as done.
mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:101
   }
+  if (isExplicitGuard(I)) {
+    auto *Cond = cast<BranchInst>(I)->getCondition();
----------------
reames wrote:
> 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))?
What is the benefit of that? When a guard looks like `br((A & B) & WC())`, it is possible to strip the widenable condition from it, getting `(A & B)`. When we have something like br((A & WC) & B)`, stripping WC would need to create instructions that don't currently exist. I would better stick to one canonical form, the more that it is easy to sustain.


================
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
----------------
reames wrote:
> Not sure this check is needed?
The logic here is a bit messy, I'll try to find a way to refactor the code around to go without it if possible.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:484
     auto *DominatedBlock = DominatedGuard->getParent();
+    if (isExplicitGuard(DominatingGuard))
+      DominatingBlock = cast<BranchInst>(DominatingGuard)->getSuccessor(0);
----------------
reames wrote:
> Having this adjust only one of the two variables looks awfully suspicious?  
We can widen *any* condition (in theory even not a condition from guard) into a dominating guard. Here we require that the dominating guard is what we are trying to support, and the dominated instruction can be anything with condition. Maybe the renaming in underlying code would make sense.


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

https://reviews.llvm.org/D56075