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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu 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:
> 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?
Yeah, when we made a change, not only we need to consider how could we make the false negative case to pass, we need to consider if the change would affect other cases, too. You are talking about the first part and we are talking about the second part. For the concrete suggestion, maybe @fhahn could help?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116754