[PATCH] D97847: Use LoopNest as the primary object on which LoopInterchange operates

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:153
+  /// of the outer loop.
+  void interchangeNestedLoops(unsigned OuterLoopId, unsigned InnerLoopId) {
+    assert((Loops[InnerLoopId]->getParentLoop() == Loops[OuterLoopId] &&
----------------
TaWeiTu wrote:
> Whitney wrote:
> > Thinking we should have a more generic set function, like setLoops?
> Agreed. But I also think having `interchangeNestedLoops` here is good in the sense that we can do the `assert`s below.
> Should I add `setLoop` in this patch?
Can we have a generic set function with a generic assert (guarded by expensive checks macro)?
like assert Loops[0] is the outermost loop, there are no other loops in between Loops[i] and Loops[i+1], Loops[Loops.size()-1] is the innermost loop, etc...

then we can have `interchangeNestedLoops` in interchange pass?


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:504
 #endif
-      Changed |= Interchanged;
+      Changed = true;
     }
----------------
TaWeiTu wrote:
> Whitney wrote:
> > why this change?
> Though unrelated to this patch, I think this makes the code a bit clearer since `Interchanged` is always `true` when we reach here, and the change seems too trivial for a separate patch.
> I can split this into another patch or just push a NFC commit of course, if you think that makes the purpose of this patch clearer.
I see it now, let's just keep it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97847