This is an archive of the discontinued LLVM Phabricator instance.

[LoopDeletion] Clear block & loop dispo cache after breaking backedge.
ClosedPublic

Authored by fhahn on Sep 26 2022, 12:16 PM.

Details

Summary

breakLoopBackedge may remove blocks and loops. Also clear block &
loop disposition to avoid the cache containing invalid blocks and loops.

Diff Detail

Event Timeline

fhahn created this revision.Sep 26 2022, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 12:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn requested review of this revision.Sep 26 2022, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 12:16 PM

While I guess this is a fix for crash/UB, was the problem covered by any tests or could we add test coverage? (or maybe the failure's so unlikely as to be impractical to test - some ABA allocation situation which rarely occurs?)

fhahn added a comment.Sep 26 2022, 1:16 PM

While I guess this is a fix for crash/UB, was the problem covered by any tests or could we add test coverage? (or maybe the failure's so unlikely as to be impractical to test - some ABA allocation situation which rarely occurs?)

Without this change, multiple transform tests fail when LLVM is built with ASAN, so this should be covered by existing tests + buildbots running ASAN.

While I guess this is a fix for crash/UB, was the problem covered by any tests or could we add test coverage? (or maybe the failure's so unlikely as to be impractical to test - some ABA allocation situation which rarely occurs?)

Without this change, multiple transform tests fail when LLVM is built with ASAN, so this should be covered by existing tests + buildbots running ASAN.

Ah, sounds good to me, then (though I'll leave it to other folks more familiar with the code to approve) - maybe a link to/quote from one of those failures would be good to include in the commit message.

nikic accepted this revision.Sep 27 2022, 12:57 AM

LGTM

This revision is now accepted and ready to land.Sep 27 2022, 12:57 AM
This revision was landed with ongoing or failed builds.Sep 30 2022, 3:22 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Sep 30 2022, 3:23 AM

Ah, sounds good to me, then (though I'll leave it to other folks more familiar with the code to approve) - maybe a link to/quote from one of those failures would be good to include in the commit message.

I added a note about how this is covered to the commit message but didn't have a link to a specific failure in buildbot handy.

Hello,

I wrote
https://github.com/llvm/llvm-project/issues/58136
about a failed asseretion with -verify-scev that starts happening with this patch.

Hello @fhahn ,

I open https://github.com/llvm/llvm-project/issues/58454 for a new failed assertion on SCEV verify in LoopUnrollAndJam, starting with this patch.
Thanks!