[PATCH] D71539: [SCEV] Look through trivial PHIs.

Fri Oct 1 14:17:51 PDT 2021

bmahjour added a subscriber: Meinersbur.
bmahjour added a comment.

In D71539#3029720 <https://reviews.llvm.org/D71539#3029720>, @fhahn wrote:

> In D71539#3028807 <https://reviews.llvm.org/D71539#3028807>, @bmahjour wrote:
>
>> `DependenceInfo::classifyPair` in DA incorrectly classifies the subscripts in the above test case as SIV, where it used to classify them as non-linear (because one of the subscripts was not an AddRec). In theory we should be able to threat them as MIV I think.
>
> Thanks for the test-case. Note that the same issue already exists without the patch if uses of the LCSSA phi node `%c.1.lcssa` are replace by its incoming value in the shared test case.

Yes, it seems that we've assumed LCSSA form for correct functioning of the DA implementation.

> The issue seems to be that `DependenceInfo::classifyPair` does not properly account for the fact that we could have AddRecs from sibling loops. It only checks the depth of the the loop of the AddRec, which means we incorrectly determine that 2 AddRecs are linear induction variables for the same loop AFAICT.

It does account for sibling loops, but the situation here is that one of the memory access instructions appears outside of the sibling loops while the subscript of that access has a recurrence with one of the sibling loops. The implementation uses a numbering scheme to distinguish the loop for the src of the dependence from the loop for the dest, but that numbering scheme gets messed up when we have a recurrence with a loop that does not contain the access instruction. I've explained the issue and proposed two possible ways to address it under D110972 <https://reviews.llvm.org/D110972> and D110973 <https://reviews.llvm.org/D110973>.

> Shouldn't we consider AddRecs from sibling loops as invariant?

Please see D110972 <https://reviews.llvm.org/D110972> and D110973 <https://reviews.llvm.org/D110973>. I suggest we agree on one of these (or another alternative) before recommiting this patch. FYI @fhahn and @Meinersbur .

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71539