[PATCH] D71689: Improve that the necessary and sufficient combination is not tried for multiple loops in loop-interchange

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Nice patch, thanks!

Some inline silly comments, but I have two mains comments.

1

Today, we drop the search at the first sign of trouble. That's a bit silly, as you stated in your introduction.

The new pattern is to only drop if it's illegal, but continue if it's not-profitable. I agree that this is guaranteed to be equal or better than the current scenario, result-wise. So, as is, looks like a clear improvement.

But in the case where a loop interchange will make access illegal, it will still drop the ball (if changed) in whatever the previous state was, which is not necessarily more profitable than the original case.

A future patch could record the transformations in a list (after checking for both legality and profitability) and only transform if the final state is more profitable than the original.

As it stands, the profitability check is too naive to do any comparison, so that would also have to improve.

2

The algorithm will now run for all legal combinations of the nesting structure, which is clearly equal or worse in terms of compiler time.

This is obviously expected, but it's still good to understand the impact. The combinations are exponential, but the number of nesting levels is usually small, so perhaps, not a big deal.

Maybe, as a safety measure (for pathological cases), we could limit the number of nesting loops we're allowed to inspect, just to be on the safe side.

Overall, if people nest 100 loops (hadouken code), they don't deserve to have their codes optimised. :D



================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:54
+enum LoopInterchangeResult {
+  NotLegal,      // Loops are not legal to interchange.
+  NotProfitable, // Loops are not profitable to interchange.
----------------
fhahn wrote:
> Could you submit this change as a separate one? It seems unrelated to changing the strategy for interchanging. Also, could you make LoopInterchangeResult an `enum class`?
I came from that path to this one, and before seeing this I was wondering why would we want that change. Looking at this one, it seems obvious.


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:544
+      for (unsigned CurrInnerLoopId = SelecLoopId;
+           (CurrInnerLoopId - NumSearchEndLoops) > 0; CurrInnerLoopId--) {
+
----------------
    CurrInnerLoopId < NumSearchEndLoops

looks simpler


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:551
+	// If the loop is not interchanged due to legality,
+	// the loop interchange is not continued.
+        // If it's a problem with profitability, try another combination.
----------------
Formatting issues?


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

https://reviews.llvm.org/D71689