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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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:
> > 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?
So to support the loop-not-rotated-case here, we could check if it can trivially rotated (header is the only exiting block, all instructions in header can moved/duplicated). 

Then proceed with the analysis. If we can replace the loop, we need to rotate the loop before we perform a transform.  To start with, we should only rotate if the loop will go away entirely, i.e. it only contains memory operations that will be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116754