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

[Ta-Wei Tu via Phabricator via llvm-commits]

TaWeiTu 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] &&
----------------
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?


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:504
 #endif
-      Changed |= Interchanged;
+      Changed = true;
     }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97847