This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Redesign of PreserveCFG Checker
ClosedPublic

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
112–113

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

137–138

not necessary anymore

llvm/lib/Passes/StandardInstrumentations.cpp
1029

nit: move parameter name to the left?

1068

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
1078

Result -> GraphBefore

1080

why do you check before pass runs?

1085

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
1021–1023

This is already public

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

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

llvm/lib/Passes/StandardInstrumentations.cpp
1068

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

1080

removed

1085

PassStack is used only inside asserts

1103

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
137–138

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
137–138

sure

yrouban updated this revision to Diff 333825.Mar 29 2021, 5:20 AM

just rebased. please review

kuhar added a comment.Mar 29 2021, 7:32 AM

Just two nits from me. I think it looks fine, but I'm not familiar with the new pass manager and don't feel confident enough to approve it.

llvm/include/llvm/Passes/StandardInstrumentations.h
137–138

I don't think this got fixed.

llvm/lib/Passes/StandardInstrumentations.cpp
1106

nit: can you put the commented argument name before the argument?

yrouban marked 2 inline comments as done.Mar 29 2021, 8:52 PM

Just two nits from me. I think it looks fine, but I'm not familiar with the new pass manager and don't feel confident enough to approve it.

Jakub, thank you for comments. I expect @skatkov or @fedor.sergeev to look from NewPM point of view.

yrouban updated this revision to Diff 334042.Mar 29 2021, 8:55 PM

fixed the nits

yrouban added inline comments.Mar 31 2021, 7:08 PM
llvm/include/llvm/Passes/StandardInstrumentations.h
419

fix registeredenabled

@skatkov, @fedor.sergeev, could you please review. During the long time this features is on the reconstruction we introduced a new bug in the LoopFlatten pass. See the comment https://reviews.llvm.org/D90940#inline-938253.

First iteration: style comments.

llvm/include/llvm/Passes/StandardInstrumentations.h
122–123

redundant {}
consider using any_of

llvm/lib/Passes/StandardInstrumentations.cpp
1017

Describe the idea of this analisys

Consider landing tests update for "-verify-cfg-preserved=0" in a separate commit. This will significantly reduce the size of review.

yrouban updated this revision to Diff 335221.Apr 5 2021, 2:02 AM
yrouban marked 2 inline comments as done.

Addressed comments.
Some tests were moved a separate patch (D99878).

skatkov accepted this revision.Apr 5 2021, 3:53 AM

with two nits.

llvm/include/llvm/Passes/StandardInstrumentations.h
419

Not Done.

llvm/lib/Passes/StandardInstrumentations.cpp
1079

Add a comment that you caches the CFG before pass.

This revision is now accepted and ready to land.Apr 5 2021, 3:53 AM
yrouban marked 2 inline comments as done.Apr 5 2021, 10:34 PM
This revision was landed with ongoing or failed builds.Apr 5 2021, 10:36 PM
This revision was automatically updated to reflect the committed changes.