[NPM] Properly reset parent loop after loop passes

[TaWeiTu]

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=49185

When NDEBUG is not set, LPMUpdater checks if the added loops have the same parent loop as the current one in addSiblingLoops.
If multiple loop passes are executed through LoopPassManager, U.ParentL will be the same across all passes.
However, the parent loop might change after running a loop pass, resulting in assertion failures in subsequent passes.

This patch resets U.ParentL after running individual loop passes in LoopPassManager.

[TaWeiTu]

Copy reproducer from bugzilla.
Will try to minimize it later.

[uabelho]

llvm/lib/Transforms/Scalar/LoopPassManager.cpp

117

I don't know this code at all, but I have to say I feel a little bit uneasy about a bugfix that only adds stuff guarded by #ifndef NDEBUG.
I'm not saying it's the case here because I don't know at all, but this *could* mean that we only fix the problem when compiling with asserts, and then in a production build the bug still exist.

Would it hurt much to always set the parent loop so it's worth putting it in an ifndef?

[TaWeiTu]

llvm/lib/Transforms/Scalar/LoopPassManager.cpp

117

Thanks for the comment!
My thought is that this is a bug of the extensive check in debug mode and not a one in the pass itself or in the overall pass management pipeline, so the fix only applies in non-release mode.
I might be wrong though. Let's see what others think first.

[asbirlea]

Fix looks good. As a detail, perhaps move the NDEBUG inside the body of setParentLoop to keep the code cleaner?

[ychen]

Could the test be simplified (using llvm-reduce) ?

[TaWeiTu]

• Move #ifndef NDEBUG guard
• Reduce bug reproducer

[ychen]

LGTM

llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

void markLoopAsDeleted(Loop &L, llvm::StringRef Name) {
     LAM.clear(L, Name);
     assert((&L == CurrentL || CurrentL->contains(&L)) &&
            "Cannot delete a loop outside of the "
            "subloop tree currently being processed.");
     if (&L == CurrentL)
       SkipCurrentLoop = true;
   }
 
+  void setParentLoop(Loop *L) {
+#ifndef NDEBUG
+    ParentL = L;
+#endif
+  }
+
   /// Loop passes should use this method to indicate they have added new child
   /// loops of the current loop.
   ///
   /// \p NewChildLoops must contain only the immediate children. Any nested
   /// loops within them will be visited in postorder as usual for the loop pass
   /// manager.
   void addChildLoops(ArrayRef<Loop *> NewChildLoops) {
     assert(!LoopNestMode &&

llvm/lib/Transforms/Scalar/LoopPassManager.cpp

for (size_t I = 0, E = IsLoopNestPass.size(); I != E; ++I) {
 
     // Finally, we intersect the final preserved analyses to compute the
     // aggregate preserved set for this pass manager.
     PA.intersect(std::move(*PassPA));
 
     // Check if the current pass preserved the loop-nest object or not.
     IsLoopNestPtrValid &= PassPA->getChecker<LoopNestAnalysis>().preserved();
 
+    // After running the loop pass, the parent loop might change and we need to
+    // notify the updater, otherwise U.ParentL might gets outdated and triggers
+    // assertion failures in addSiblingLoops and addChildLoops.
+    U.setParentLoop(L.getParentLoop());
+
     // FIXME: Historically, the pass managers all called the LLVM context's
     // yield function here. We don't have a generic way to acquire the
     // context and it isn't yet clear what the right pattern is for yielding
     // in the new pass manager so it is currently omitted.
     // ...getContext().yield();
   }
   return PA;
 }
for (auto &Pass : LoopPasses) {
     // Update the analysis manager as each pass runs and potentially
     // invalidates analyses.
     AM.invalidate(L, *PassPA);
 
     // Finally, we intersect the final preserved analyses to compute the
     // aggregate preserved set for this pass manager.
     PA.intersect(std::move(*PassPA));
 
+    // After running the loop pass, the parent loop might change and we need to
+    // notify the updater, otherwise U.ParentL might gets outdated and triggers
+    // assertion failures in addSiblingLoops and addChildLoops.
+    U.setParentLoop(L.getParentLoop());
+
     // FIXME: Historically, the pass managers all called the LLVM context's
     // yield function here. We don't have a generic way to acquire the
     // context and it isn't yet clear what the right pattern is for yielding
     // in the new pass manager so it is currently omitted.
     // ...getContext().yield();
   }
   return PA;
 }

llvm/test/Transforms/Util/pr49185.ll

+; REQUIRES: asserts
+; RUN: opt -S -passes='default<O3>' %s
+%struct0 = type { i64, i64, i32, i64, i32 }
+%struct1 = type { i32 }
+%union0 = type { i32 }
+%union1 = type { i16 }
+%union2 = type { i32 }
+
+@g_6 = external dso_local global i32, align 1
+@g_60 = external dso_local global i16, align 1
+@g_79 = external dso_local global { i16, i16 }, align 1
+@g_315 = external dso_local global %struct0, align 1
+@g_359 = external dso_local global %struct0, align 1
+
+define dso_local i16 @main(i16 %argc, i16** %argv) #0 {
+entry:
+  %call2 = call i16 @func_1()
+  unreachable
+}
+
+define internal i16 @func_1() #0 {
+entry:
+  %call = call i16 @func_21(i32* undef, i32 undef, i32* undef)
+  ret i16 undef
+}
+
+define internal i16 @func_21(i32* %p_22, i32 %p_23, i32* %p_24) #0 {
+entry:
+  call void @func_34(%struct0* align 1 undef, i32 undef, i32 undef, i32* @g_6, %union0* byval(%union0) align 1 undef)
+  unreachable
+}
+
+define internal void @func_34(%struct0* %agg.result, i32 %p_35, i32 %p_36, i32* %p_37, %union0* %p_38) #0 {
+entry:
+  %p_37.addr = alloca i32*, align 1
+  %cleanup.dest.slot = alloca i32, align 1
+  store i32* %p_37, i32** %p_37.addr, align 1
+  br label %lbl_898
+
+lbl_898:                                          ; preds = %cleanup3097, %entry
+  br label %lbl_1111
+
+lbl_1111:                                         ; preds = %cleanup3097, %lbl_898
+  %0 = load i32, i32* getelementptr inbounds (%struct0, %struct0* @g_359, i32 0, i32 4), align 1
+  %tobool1833 = icmp ne i32 %0, 0
+  br i1 %tobool1833, label %land.rhs1834, label %land.end1851
+
+land.rhs1834:                                     ; preds = %lbl_1111
+  store i16 0, i16* @g_60, align 1
+  br label %land.end1851
+
+land.end1851:                                     ; preds = %land.rhs1834, %lbl_1111
+  %1 = load i32*, i32** %p_37.addr, align 1
+  %2 = load i32, i32* %1, align 1
+  %tobool2351 = icmp ne i32 %2, 0
+  br i1 %tobool2351, label %if.then2352, label %if.else3029
+
+if.then2352:                                      ; preds = %land.end1851
+  %3 = load i16, i16* getelementptr inbounds ({ i16, i16 }, { i16, i16 }* @g_79, i32 0, i32 0), align 1, !tbaa !1
+  %tobool3011 = icmp ne i16 %3, 0
+  call void @llvm.assume(i1 %tobool3011)
+  store i32 11, i32* %cleanup.dest.slot, align 1
+  br label %cleanup3097
+
+if.else3029:                                      ; preds = %land.end1851
+  store i32 3, i32* getelementptr inbounds (%struct0, %struct0* @g_315, i32 0, i32 4), align 1
+  store i32 132, i32* %cleanup.dest.slot, align 1
+  br label %cleanup3097
+
+cleanup3097:                                      ; preds = %if.else3029, %if.then2352
+  %cleanup.dest3113 = load i32, i32* %cleanup.dest.slot, align 1
+  switch i32 %cleanup.dest3113, label %cleanup3402 [
+    i32 132, label %lbl_1111
+    i32 11, label %lbl_898
+  ]
+
+cleanup3402:                                      ; preds = %cleanup3097
+  ret void
+}
+
+; Function Attrs: nofree nosync nounwind willreturn
+declare void @llvm.assume(i1 noundef) #4
+
+attributes #0 = { "use-soft-float"="false" }
+attributes #1 = { argmemonly nofree nosync nounwind willreturn }
+attributes #2 = { noinline }
+attributes #3 = { argmemonly nofree nosync nounwind willreturn writeonly }
+attributes #4 = { nofree nosync nounwind willreturn }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 13.0.0"}
+!1 = !{!2, !2, i64 0}
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C/C++ TBAA"}