[PATCH] D99149: [LoopUnrollAndJam] Change LoopUnrollAndJamPass to LoopNest pass

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopPassManager.h:255
   void markLoopAsDeleted(Loop &L, llvm::StringRef Name) {
-    assert((!LoopNestMode || L.isOutermost()) &&
+    assert((!LoopNestMode || CurrentL == &L) &&
            "L should be a top-level loop in loop-nest mode.");
----------------
uint256_t wrote:
> Whitney wrote:
> > uint256_t wrote:
> > > I don't know if this change is ok, but I think CurrentL is always a top-level loop if it's loop nest mode.
> > If CurrentL is always a top-level loop if it's loop nest mode, then why do we need to change from `L.isOutermost()` to `CurrentL == &L`?
> I found that `markLoopAsDeleted` is sometimes called on loops that are already destroyed (e.g. by `LI->erase(L)`). Then `L.isOutermost()` is invalid since the destructor of `L` is called. (asan detected it)
> 
```
   /// This runs the destructor of the loop object making it invalid to
   /// reference afterward. The memory is retained so that the *pointer* to the
   /// loop remains valid.
   destroy()
```
So I agree `L.isOutermost()` should be changed.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:52
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Scalar/LoopPassManager.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
----------------
Is this change still needed?


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

https://reviews.llvm.org/D99149