Page MenuHomePhabricator

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

Authored by aeubanks on Mon, Mar 13, 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.Mon, Mar 13, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 13, 5:11 PM
aeubanks requested review of this revision.Mon, Mar 13, 5:11 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMon, Mar 13, 5:11 PM
aeubanks edited the summary of this revision. (Show Details)Mon, Mar 13, 5:13 PM
aeubanks edited the summary of this revision. (Show Details)Mon, Mar 13, 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.Tue, Mar 14, 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.Tue, Mar 14, 2:23 AM
aeubanks updated this revision to Diff 505169.Tue, Mar 14, 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.Tue, Mar 14, 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.Tue, Mar 14, 2:12 PM
aeubanks updated this revision to Diff 505532.Wed, Mar 15, 9:27 AM
aeubanks edited the summary of this revision. (Show Details)

rebase after cleanups
fix tests

aeubanks added inline comments.Wed, Mar 15, 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.Wed, Mar 15, 9:32 AM
This revision was landed with ongoing or failed builds.Wed, Mar 15, 1:17 PM
This revision was automatically updated to reflect the committed changes.