[PATCH] D123400: [LoopCacheAnalysis] Consider dimension depth of the subscript reference when calculating cost

Tue Apr 12 17:59:26 PDT 2022

congzhe accepted this revision.
congzhe added a comment.

In D123400#3443913 <https://reviews.llvm.org/D123400#3443913>, @bmahjour wrote:

> In D123400#3442881 <https://reviews.llvm.org/D123400#3442881>, @amehsan wrote:
>
>> 1. Boiling down various parameters to one final "cost" value has a disadvantage. In fact we discussed this with @congzhe as the first solution, but we discarded it because essentially you lose some informaiton when you boil everything down to one number. The reason that Congzhe decided to go with a two component cost was that extra detail, carries more useful information and helps to make more accurate decision. Now this was a theoretical concern and I am not sure if in practice it holds or not. But that is the background on congzhe's work.
>
> The cost returned by the analysis must consider the impact of outer dimension strides incurred by each reference group, otherwise it will not be accurate. If, for whatever reason, we need extra details in future we could provide extra interfaces to the analysis to query those details.
>
>> 1. Wouldn't it be more appropriate to continue discussion on congzhe's patch rather than posting a parallel patch to address the same problem? I think that will be more constructive given congzhe has already identified the problem, investigated some solutions and developed a patch? I suggest we continue the discussion, as review comments on his patch. Once there is an agreement on the cost function, Congzhe can update the patch (assuming it is needed) to reflect the agreed upon solution.
>
> I explained my reasoning for needing a different approach in D122776 <https://reviews.llvm.org/D122776>. I just figured it would be a lot easier to show what I mean by posting a new patch instead of trying to describe it as a change request on top of D122776 <https://reviews.llvm.org/D122776>. I'm happy to continue further discussion on this wherever seems more appropriate to you or other reviewers.

Thanks Bardia for this work, I think the approach in this patch does resolve the motivating problem described in D122776 <https://reviews.llvm.org/D122776>. Nevertheless, I would also like to clarify that my approach in D122776 <https://reviews.llvm.org/D122776> does consider outer dimension strides incurred by each reference group. Currently what loop cache analysis does is that for each loop we sum all costs of all reference groups and get the final cost (which is what your patch does as well). What I did is to take the stride into account as a second component, and for each loop take the maximum stride of all reference groups to get the final stride, which presumably could resolve the motivating problem too.

After you land this patch, I hope that I could get the test case in D122776 <https://reviews.llvm.org/D122776> merged, since that is really the motivating test for these works. I could update the "CHECK: " lines according to the approach proposed in this patch, and update D122776 <https://reviews.llvm.org/D122776> to a pure NFC patch with that test only. I look forward to your thoughts about it :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123400