[PATCH] D124926: [LoopInterchange] New cost model for loop interchange

Fri Jun 3 07:35:42 PDT 2022

congzhe added a comment.

In D124926#3555877 <https://reviews.llvm.org/D124926#3555877>, @bmahjour wrote:

>> Having all tests depend on a single target is not ideal, because this limits the number of bots/configurations that run the tests. I think we need a way to keep most of the tests independent of the PowerPC or any other target and only have tests that explicitly test the cost-modeling depend on the target.
>
> The analysis uses `getCacheLineSize()` which returns 0 by default unless a subtarget overrides it. I think value 0 is meant for machines with no cache model (in which case loop interchange will likely not matter). Ideally all other targets should implement this function, but for the time being two ways I can think of to solve this without making the tests target-dependent are:
>
> 1. Define an option-controlled CacheLineSize inside `LoopCacheAnalysis.cpp` (with a default of say 64). If the option is specified we use the value supplied otherwise, we call `getCacheLineSize()` and use the value if it's non zero, or fall-back to the default value in case of zero.
> 2. Halt the analysis and invalidate the result when `getCacheLineSize()` returns zero. Clients must check the validity of result before using it, or query if the analysis can be computed for the specified target. This looks undesirable to me, since it makes the analysis less consumable. Also there seem to be targets that can benefit from interchange  but just haven't implemented `getCacheLineSize()` yet.

Thanks Florian for finding the cause, and thanks Bardia for your suggestions. On this thread I think I can try your solution #1 first. I remember I actually tried exactly this approach weeks ago internally and it worked (although I never posted it). Let me take a second look and if it does work fine, I'll update my patch accordingly and reset the target triple? For the ones that had x86 previously I can reset it to x86, and for the ones that do not have a target triple I can remove it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124926