This is an archive of the discontinued LLVM Phabricator instance.

[StandardInstrumentations] Verify function doesn't change if analyses are preserved
ClosedPublic

Authored by aeubanks on Mar 13 2023, 5:11 PM.

Details

Summary

Reuse StructuralHash and allow it to be used in non-expensive checks builds.

Move PreservedAnalysisChecker further down StandardInstrumentations so other Instrumentations (e.g. printing) have a chance to run before PreservedAnalysisChecker crashes.

Diff Detail

Event Timeline

aeubanks created this revision.Mar 13 2023, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2023, 5:11 PM
aeubanks requested review of this revision.Mar 13 2023, 5:11 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 13 2023, 5:11 PM
aeubanks edited the summary of this revision. (Show Details)Mar 13 2023, 5:13 PM
aeubanks edited the summary of this revision. (Show Details)Mar 13 2023, 5:15 PM

Looks good in general, but it would be better to have this patch split into several smaller patches: renaming NFC, InstructionCounter intro, one for each particular pass fix.

llvm/lib/Passes/StandardInstrumentations.cpp
1076

may be rename to Value?

2201

I think the time profiling pass has to be the last one.

nikic requested changes to this revision.Mar 14 2023, 2:23 AM

Please split out the fixes to individual passes.

llvm/lib/Passes/StandardInstrumentations.cpp
46–47

Better move the default behind EXPENSIVE_CHECKS.

1072

Instead of instruction count, it would be better to use StructuralHash. It exists for exactly this purpose.

llvm/lib/Transforms/Coroutines/CoroCleanup.cpp
133 ↗(On Diff #504895)

Can preserve CFG here.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
568 ↗(On Diff #504895)

Can preserve CFG here.

616 ↗(On Diff #504895)

Should return from salvageKnowledge whether something was salvaged...

This revision now requires changes to proceed.Mar 14 2023, 2:23 AM
aeubanks updated this revision to Diff 505169.Mar 14 2023, 10:55 AM
aeubanks retitled this revision from [StandardInstrumentations] Check that the number of instructions doesn't change if analyses are preserved to [StandardInstrumentations] Verify function doesn't change if analyses are preserved.
aeubanks edited the summary of this revision. (Show Details)

split out other changes, use StructuralHash instead

nikic requested changes to this revision.Mar 14 2023, 2:12 PM

nontrivial-unswitch-markloopasdeleted.ll failure looks legit.

llvm/lib/Passes/StandardInstrumentations.cpp
1121–1131

Hm, aren't we going to skip the CFG check below if only CFGAnalyses are preserved (but not AllAnalyses)?

This revision now requires changes to proceed.Mar 14 2023, 2:12 PM
aeubanks updated this revision to Diff 505532.Mar 15 2023, 9:27 AM
aeubanks edited the summary of this revision. (Show Details)

rebase after cleanups
fix tests

aeubanks added inline comments.Mar 15 2023, 9:28 AM
llvm/lib/Passes/StandardInstrumentations.cpp
1121–1131

no longer a worry after D146096

This revision is now accepted and ready to land.Mar 15 2023, 9:32 AM
This revision was landed with ongoing or failed builds.Mar 15 2023, 1:17 PM
This revision was automatically updated to reflect the committed changes.