[PATCH] D116754: Bugfix for application of trivial loop optimization in LoopIdiomRecognize(Github Issue #50308)

[Raghu R via Phabricator via llvm-commits]

razetime added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:628-636
   // We can only promote stores in this block if they are unconditionally
-  // executed in the loop.  For a block to be unconditionally executed, it has
-  // to dominate all the exit blocks of the loop.  Verify this now.
-  for (BasicBlock *ExitBlock : ExitBlocks)
-    if (!DT->dominates(BB, ExitBlock))
-      return false;
+  // executed in the loop. For a block to be unconditionally executed, it has
+  // to dominate all the exit blocks of the loop. Verify this now.
+  // On countable unrotated loops, checking the exit blocks fails.
+  // Hence, this is only run on rotated loops.
+  if (isFromRotatedLoop)
+    for (BasicBlock *ExitBlock : ExitBlocks)
----------------
ChuanqiXu wrote:
> razetime wrote:
> > lebedev.ri wrote:
> > > I do not understand why we can just not do this check for unrotated loops?
> > The check unconditionally failed on the unrotated test case I was using from https://llvm.godbolt.org/z/YrGqGhnE9.
> > 
> > The place where it failed was when BB =`loop.latch` and ExitBlock = `exit`. pre-rotating the loop would work, but it seemed unintuitive and costlier than skipping the check. 
> It might not be a good reason to remove a check simply if it fails on a false negative condition. Since the condition might do many other true negative checks.
This is the dominator tree for the  given IR file:

{F21496301}

The problem here is that `loop.latch` cannot dominate `exit` since they're on the same level i.e. this check returns false:

>From llvm/include/llvm/Support/GenericDomTree.h

```
    // A can only dominate B if it is higher in the tree.
    if (A->getLevel() >= B->getLevel()) return false;
```

I'm not sure how this check can be modified to accept the IR for optimization. Maybe a completely different check should be added?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116754