[PATCH] D94717: [LoopNest] Consider loop nest with inner loop guard using outer loop induction variable to be perfect

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:65
   /// (if there are any). Return the last basic block found or \p End if it was
   /// reached during the search.
   static const BasicBlock &skipEmptyBlockUntil(const BasicBlock *From,
----------------
Please update the description.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:405
+  // otherwise return nullptr.
+  if (&LoopNest::skipEmptyBlockUntil(ExitFromLatchSucc, GuardOtherSucc) ==
+      GuardOtherSucc)
----------------
bmahjour wrote:
> Whitney wrote:
> > sidbav wrote:
> > > bmahjour wrote:
> > > > AFAICS `skipEmptyBlockUntil` doesn't check that the unique successor blocks have unique predecessors. Don't we need to check for that too?
> > > Yes, I intended on adding adding it, but I ran into LIT test issues so I did not put those changes in the patch.... Just took another look at it, and I realized I made a mistake in the initial implementation. It is working now. 
> > I thought about that too, but it seems to be questionable...
> > 
> > ```
> > if (cond)
> >   goto label;
> > if (0 < N) {
> >   for (int i = 0; i < N; ++i) {...}
> > label:  
> > }
> > ```
> > Should we consider `if (0 < N)` to be a loop guard? It actually guarded the loop, but it is not a single entry single exit region.
> > 
> > 
> > If we decided to check for unique predecessor, it may make sense to do it in `skipEmptyBlockUntil`.
> I think we should keep the control-flow cases that are considered "guard-like" fairly simple (otherwise transforms will be faced with too many canonical forms to have to deal with). I'd say we do not consider `if (0 < N)` in the example above as a loop guard, given that `label:` is also being guarded by that condition.
> 
> There may be legitimate use cases for the current semantics of `skipEmptyBlockUntil`, so I think we should create another version of that function (or pass a flag to it) to additionally check for existence of unique predecessors. Then we could use that version of the function here to decide whether the control flow structure should be considered a guard. @sidbav could you please do that?
@sidbav Can you please add a unit test in llvm/unittests/Analysis/LoopInfoTest.cpp for a test case like
```
if (cond)
  goto label;
if (0 < N) {
  for (int i = 0; i < N; ++i) {...}
label:  
}
```
where the branch `if (0 < N)` should be not a guard.


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

https://reviews.llvm.org/D94717