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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D124926#3529179 <https://reviews.llvm.org/D124926#3529179>, @eopXD wrote:

>> There's some changes to lit tests. Most of them are minor. One change that applies to all tests is that I changed the target triple from "x86_64" to "aarch64" and added "-mcpu=tsv110" in the RUN lines.
>
> IIUC, this means that `IndexInner = CostMap.find(InnerLoop)->second = IndexOuter = CostMap.find(OuterLoop)->second = 0` here, right?

Thanks for the comment!

With "x86_64" target triple since TTI->getCachelineSize() returns 0, loop cache analysis would execute analysis incorrectly, which means the loop vector it output may not be correct. What this indicates in loop interchange is that, `IndexInner`/`IndexOuter` might be messed up such that even if `InnerLoop` should be placed as the outer loop and `OuterLoop` should be placed as the inner loop, `IndexInner` might not be smaller than `IndexOuter` and hence we won't do interchange.

> Will your change possibly take away some coverage from x86? I think having a pre-commit to fill in something in x86's TTI to make sure its working fine on the new model would be fine. We can have multiple `RUN` lines on different architectures.

IMHO I don't think we necessarily lose coverage for x86. Loop interchange is a mid-end pass that is not dependent on specific backend targets, although we need a little bit information from backends which is `CachelineSize`. Whichever "target triple" we use in our tests won't affect the functionality of this pass (and the test coverage). Regarding a pre-commit of "getCachelineSize()": I'm leaning towards leaving the implementation of `getCachelineSize()` in different backends to experts who are most familiar with that specific backend, let it be x86, arm, arc, loongArch, RISCV, etc, since it looks like implementation of "getCachelineSize()" is missing in a few targets. I'd appreciate it if you could let me know your thoughts.


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

https://reviews.llvm.org/D124926