[PATCH] D110973: [DA] Handle mismatching loop levels by considering them non-linear

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:921-924
   if (IsSrc)
     Loops.set(mapSrcLoop(AddRec->getLoop()));
   else
     Loops.set(mapDstLoop(AddRec->getLoop()));
----------------
Meinersbur wrote:
> `mapSrcLoop`/`mapDstLoop` may already be out-of-range because `AddRec->getLoop()` is not surrounding the SCEVExpr. 
> 
> One could include a sanity check inside `mapSrcLoop`/`mapDstLoop`:
> ```
>     const Loop* L = LoopNest;
>     while (L && AddRec->getLoop() != L) 
>         L = L->getParentLoop();
>     assert (L && "Must be in loop nest, otherwise mapSrcLoop/mapDstLoop will be out-of-range");
> ```
I tried adding this assert inside `checkSubscripts` right before the calls to `mapSrcLoop` and `mapDstLoop` and was able to get your test case to assert with the previous version of this patch. I've now added your test into `MismatchingNestLevels.ll`.  

I also looked into adding this assert, but decided not to do it for the following reasons:
1. Except for `checkSubscripts`, all callers of `mapSrcLoop` and `mapDstLoop`, get their loops from an AddRec. Checking that the AddRec's loop exists in the loop nest will always satisfy the assertion, so it's pointless to add in those cases. Inside `checkSubscript` with this patch we already check for the condition being asserted and exit early, so no point in adding the assert inside `checkSubscript` either.
2. I tried adding `assert(D > 0 && D <= MaxLevels)` to `mapSrcLoop` and `mapDstLoop`, but it didn't get triggered with the previous version of this patch. That's actually expected because with the numbering system a level will never be outside of this range. The consideration for a level to be out of range depends on the specific dependence test (eg. while `exactSIVtest` asserts that `Level <= CommonLevels`, `weakZeroSrcSIVtest` asserts that `Level <= MaxLevels`), so there is no way to add such assertion to  `mapSrcLoop` and `mapDstLoop` because they are used by various kinds of dependence tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110973