Page MenuHomePhabricator

[NPM] Properly reset parent loop after loop passes
ClosedPublic

Authored by TaWeiTu on Feb 15 2021, 12:23 PM.

Details

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.

Diff Detail

Event Timeline

TaWeiTu created this revision.Feb 15 2021, 12:23 PM
TaWeiTu requested review of this revision.Feb 15 2021, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 12:23 PM
TaWeiTu updated this revision to Diff 323818.Feb 15 2021, 12:30 PM

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

uabelho added inline comments.Feb 16 2021, 12:32 AM
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 added inline comments.Feb 16 2021, 12:45 AM
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 accepted this revision.Feb 17 2021, 5:58 PM

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

This revision is now accepted and ready to land.Feb 17 2021, 5:58 PM
ychen added a comment.Feb 17 2021, 9:08 PM

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

TaWeiTu updated this revision to Diff 324544.Feb 18 2021, 12:02 AM
  • Move #ifndef NDEBUG guard
  • Reduce bug reproducer
ychen accepted this revision.Feb 18 2021, 9:29 AM

LGTM

This revision was automatically updated to reflect the committed changes.