Page MenuHomePhabricator

[NewPM] Redesign of PreserveCFG Checker
Needs ReviewPublic

Authored by yrouban on Nov 12 2020, 12:59 AM.

Details

Summary

Please see the reason for this redesign in the patch D91324.
Basically the checker introduces an internal custom CFG analysis that tracks current up-to date CFG snapshot. The analysis is invalidated along with any other CFG related analysis (the key is CFGAnalyses). If the CFG analysis is not invalidated at a functional pass exit then the checker asserts that the CFG snapshot taken from this analysis is equals to a snapshot of the current CFG.

Along the way:

  • the function CFG::printDiff() is simplified by removing function name calculation. The name is printed by the caller;
  • fixed CFG invalidated condition (see CFG::invalidate());
  • StandardInstrumentations::registerCallbacks() gets additional optional parameter of type FunctionAnalysisManager*, which is needed by the checker to get the custom CFG analysis;
  • several PM related tests updated to explicitly set -verify-cfg-preserved as they need.

This patch is safe to land as the CFGChecker is left switched off (the options -verify-cfg-preserved is false by default). It will be switched on by a separate patch to minimize possible reverts.

Diff Detail

Event Timeline

yrouban created this revision.Nov 12 2020, 12:59 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 12 2020, 12:59 AM
yrouban requested review of this revision.Nov 12 2020, 12:59 AM
kuhar added a comment.Nov 17 2020, 3:32 PM

Found some cosmetics, but I'm not familiar enough with the NPM to do a full review.

llvm/include/llvm/Passes/StandardInstrumentations.h
107–108

Consider pulling this out of CFG. I don't see many reasons for having 3 levels of class nesting.

136

not necessary anymore

llvm/lib/Passes/StandardInstrumentations.cpp
741

nit: move parameter name to the left?

771

I think this will print with errs(). Would it make sense to flush dbgs() ahead of printing with errs()?

skatkov added inline comments.Nov 17 2020, 10:43 PM
llvm/lib/Passes/StandardInstrumentations.cpp
781

Result -> GraphBefore

783

why do you check before pass runs?

787

you should not PassStack.pop_back_val() for product build?

yrouban updated this revision to Diff 306604.Nov 19 2020, 11:17 PM
yrouban marked 4 inline comments as done.
yrouban edited the summary of this revision. (Show Details)

Addressed comments.

Looks fine to me, but I'm not confident enough to give an approval.

llvm/lib/Passes/StandardInstrumentations.cpp
733–735

This is already public

yrouban added inline comments.Nov 22 2020, 9:24 PM
llvm/include/llvm/Passes/StandardInstrumentations.h
136

there can bee a need to disabled/enable (e.g. for some tests or for debugging).

llvm/lib/Passes/StandardInstrumentations.cpp
771

I believe this comment is true for all callsites of report_fatal_error(). If so it should be done inside.

783

removed

787

PassStack is used only inside asserts

805

to @skatkov
if AfterPassCallback is called before AM.invalidate() then we check CFG only if the condition (PassPA.allAnalysesInSetPreserved<CFGAnalyses>() || PassPA.allAnalysesInSetPreserved<AllAnalysesOn<Function>>()) holds.
if AfterPassCallback is called after AM.invalidate() then we check CFG only if the same condition holds and the PreservedCFGCheckerAnalysis is not invalidated. But it is invalidated according to the same condition.
All in all it does not matter if AfterPassCallback is called before or after AM.invalidate().

Looks fine to me, but I'm not confident enough to give an approval.

Thanks. I'm working closely with @skatkov and believe he have enough expertise to review fully.
The biggest issue with the current design is that we have to send FAM to the initialization of StandardInstrumentations/PreservedCFGCheckerInstrumentation. This looks asymmetric to the other analysis managers and it would be more reasonable to send FAM or all AMs to the instrumentation callbacks as parameters.

kuhar added inline comments.Nov 22 2020, 10:09 PM
llvm/include/llvm/Passes/StandardInstrumentations.h
136

I meant the 'public:'. You made everything public at the very top of the class.

yrouban added inline comments.Nov 22 2020, 10:11 PM
llvm/include/llvm/Passes/StandardInstrumentations.h
136

sure