[PATCH] D53027: [LoopInterchange] Fix inner loop reduction handling.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In https://reviews.llvm.org/D53027#1268066, @efriedma wrote:

> This analysis seems very fragile.


Agreed, it is not ideal and I think alternatively we could just drop support for such loops as it is unlikely they will be profitable  (we are moving loads/stores from outer loops to inner loops). Unfortunately I did not find any existing analysis that could be re-used for those checks, but I think they are quite conservative.

It would be great if you could let me know which option you prefer. If we go with this patch, I'll address the code comments and also add a comment in line with my responses. The underlying problem is that loops like the one below are miscompiled if they are interchanged, if y is a global variable.

  for (unsigned i = 0; i < N; i++) {
    y = 0;
    for (unsigned j = 0; j < N; j++) {
      A[i][j] += 1;
      y += A[i][j];
    }
  }



================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:715
+              continue;
+            SI = nullptr;
+          }
----------------
efriedma wrote:
> What happens if there are mutiple stores using the PHI?
We could just support a single store in the exit block for now I think.


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:722
+      if (!LI || !SI || SI->getOperand(1) != LI->getOperand(0))
+        return false;
+
----------------
efriedma wrote:
> Could any operations between the load and store alias them?  Does it matter?
I don't think aliasing operations between them (in the inner loop body) should be a problem. The store in the exit makes sure we write the correct value after the inner loop has been executed, and the load makes sure the inner loop uses the right values.

Having multiple load - reduction - stores which alias shouldn't be a problem either.  As long as each chain forms a cycle, the results should be properly be preserved between inner loop executions and we do not change the relative order of the loads/stores. If we would have multiple stores that alias, it would mean we reduce multiple chains to a single one.


https://reviews.llvm.org/D53027