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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:405
+  // otherwise return nullptr.
+  if (&LoopNest::skipEmptyBlockUntil(ExitFromLatchSucc, GuardOtherSucc) ==
+      GuardOtherSucc)
----------------
Whitney 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?
> 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?


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

https://reviews.llvm.org/D94717