This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Verify contents of cached Block Dispositions
AbandonedPublic

Authored by mkazantsev on Sep 24 2021, 12:04 AM.

Details

Summary

We have some evidence that SCEV's cached BlockDispositions are sometimes
wrong (i.e. cached result is different from what computeBlockDisposition would
return), but not clear where and how exactly the cache had gone astray.

Currently SCEV blindly believes that its cache is valid and does not bother to check
it, so such problems may lurk between passes. This patch tries to fix this situation.

Diff Detail

Event Timeline

mkazantsev created this revision.Sep 24 2021, 12:04 AM
mkazantsev requested review of this revision.Sep 24 2021, 12:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2021, 12:04 AM
mkazantsev edited the summary of this revision. (Show Details)Sep 24 2021, 12:23 AM

Looks like a good thing to verify to me, thanks! I can't tell due to missing context in the diff, are the new checks enabled by default? If so, do you think this will cause any verification failures during bootstrapping that should be addressed first?

llvm/test/Transforms/LCSSA/pr44058.ll
6

Looks like all changed tests fail verification due to caching deleted values. Are there any cases where the disposition of a value changes?

mkazantsev added a comment.EditedSep 24 2021, 1:52 AM

Looks like a good thing to verify to me, thanks! I can't tell due to missing context in the diff, are the new checks enabled by default? If so, do you think this will cause any verification failures during bootstrapping that should be addressed first?

Yes, the checks are now on by default.

All these failures are fixed in D110390. I'm going to merge them together. Further on, if other failures are found, we should treat it as a bug.

mkazantsev added inline comments.Sep 24 2021, 1:55 AM
llvm/test/Transforms/LCSSA/pr44058.ll
6

All the failures are due to dangling pointers. We don't have an API to somehow invalidate this cache, and I'm surprised it doesn't break on every step. I've tried to fix things up in D110390.

Updated revision w/ context.

mkazantsev planned changes to this revision.Oct 6 2021, 4:14 AM
mkazantsev updated this revision to Diff 377839.Oct 7 2021, 7:21 AM
mkazantsev edited the summary of this revision. (Show Details)
mkazantsev added a reviewer: bjope.
mkazantsev added a subscriber: bjope.

Updated.

  • Now we ignore SCEVs that contain dangling pointers.
  • Thanks @bjope for finding an example where it ACTUALLY cathes a problem, see D110813. Added this as unit test.
  • Printing block name instead of entire block dump;
  • Printing smart block dispositions as string instead of ints.
mkazantsev updated this revision to Diff 378140.Oct 8 2021, 2:28 AM
mkazantsev planned changes to this revision.Oct 8 2021, 3:39 AM

Rebased. Now, when we wipe out obsolete data from caches, this is not failing. The only failure seems to be a real bug in LoopFusion.

mkazantsev edited the summary of this revision. (Show Details)Oct 11 2021, 3:28 AM
mkazantsev planned changes to this revision.Oct 22 2021, 12:43 AM

Needs rebase. Withdrawing from review for now.

This one is now ready for review. Please feel free to take a look.

bjope added inline comments.Nov 24 2021, 12:15 AM
llvm/test/Transforms/LoopFusion/triple_loop_nest_inner_guard.ll
10

I figure this is a fault that we now detect that should be tracked somehow. Should there be a reference to a bugzilla issue here?

FYI, the issue fixed in D114738 could cause this check to fail. As such, this is blocked on that patch.

llvm/test/Transforms/LoopFusion/triple_loop_nest_inner_guard.ll
10

This to me looks to be a blocker. We must understand where this failure is coming from and fix it before landing the verification.

reames requested changes to this revision.Nov 29 2021, 1:59 PM

Please refresh once XFAIL root caused and fixed.

This revision now requires changes to proceed.Nov 29 2021, 1:59 PM
mkazantsev abandoned this revision.Feb 13 2022, 11:01 PM

No plan to make further progress here.