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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:508
+    // Obtain the loop vector returned from loop cache analysis beforehand,
+    // and populate the <Loop, index> pair into a map for constant time query
+    // later. Indices in loop vector reprsent the optimal order of the
----------------
[nit] `and populate the <Loop, index> pair into a map` -> `and put each <Loop, index> pair into a map...`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:513
+    // indicates the loop should be placed as the innermost loop, .
+    const auto &LoopCosts = CC->getLoopCosts();
+    DenseMap<const Loop *, unsigned> CostMap;
----------------
Since `CC` can be nullptr we need to assert that that's not the case.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1165
+  // versa.
+  unsigned IndexInner = 0, IndexOuter = 0;
+  if (CostMap.find(InnerLoop) != CostMap.end() &&
----------------
[nit] IndexInner -> InnerIndex
[nit] IndexOuter -> OuterIndex


================
Comment at: llvm/test/Transforms/LICM/lnicm.ll:6
 ; This test represents the following function:
-; void test(int x[10][10], int y[10], int *z) {
-;   for (int k = 0; k < 10; k++) {
+; void test(int x[m][n], int y[n], int *z) {
+;   for (int k = 0; k < n; k++) {
----------------
[nit] please add `n` and `m` as parameter. ie `void test(int n, int m, int x[m][n], int y[n], int *z) {`


================
Comment at: llvm/test/Transforms/LICM/lnicm.ll:20
 
-define dso_local void @test([10 x i32]* noalias %x, i32* noalias readonly %y, i32* readonly %z) {
+define dso_local void @test(ptr noalias %x, ptr noalias readonly %y, ptr readonly %z, i64 %m, i64 %n) {
 ; CHECK-LABEL: @test(
----------------
[nit] correct order based on the C code above.


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

https://reviews.llvm.org/D124926