[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)
----------------
razetime wrote:
> 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?
Since the IR optimization is incorrect, is pre-rotating the loop a valid strategy here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116754