Page MenuHomePhabricator

[NewPM] Disable PreserveCFGChecker and add regression unit tests
ClosedPublic

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

Details

Summary

The design of the PreserveCFG Checker (landed as D81558 [NewPM] Introduce PreserveCFG check) has a fundamental flaw which makes it incorrect. The checker is based on the PreservedAnalyses result returned by functional passes: if CFGAnalyses is in the returned PreservedAnalyses set, then the checker asserts that the CFG snapshot saved before the pass is equal to the CFG snapshot taken after the the pass. The problem is in passes that change CFG and invalidate CFGAnalyses on their own. Such passes do not return CFGanalyses in the returned PreservedAnalyses. So the checker mistakenly expects CFG unchanged. As an example see the class TestSimplifyCFGInvalidatingAnalysisPass in the new tests.

It is interesting that the bug was not found in LLVM. That is because the CFG checker ran only if CFGAnalyses was checked incorrectly:

if (!PassPA.allAnalysesInSetPreserved<CFGAnalyses>())
  return;

but must be checked as follows:

auto PAC = PA.getChecker<PreservedCFGCheckerAnalysis>();
if (!(PAC.preserved() || PAC.preservedSet<AllAnalysesOn<Function>>() || PAC.preservedSet<CFGAnalyses>())
  return;

This fix and a fully redesigned checker will be sent as a separate follow-up patch.

Diff Detail

Event Timeline

yrouban created this revision.Nov 12 2020, 12:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 12:47 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
yrouban requested review of this revision.Nov 12 2020, 12:47 AM
skatkov accepted this revision.Nov 16 2020, 8:23 PM

Do I understand correctly that with -verify-cfg-preserved=true all new tests fail?

I would suggest to move new tests to separate test file. Mark it as expected to fail and do not change the value of -verify-cfg-preserved in this patch.
Then in a separate commit, turn the option be default and remove expected fail.
After that fix the issue and turn the option to on back.

If it is possible actually :)

llvm/unittests/IR/PassManagerTest.cpp
811

could you please be more precise. Specifically state explicitly what is "accordingly".

This revision is now accepted and ready to land.Nov 16 2020, 8:23 PM

Do I understand correctly that with -verify-cfg-preserved=true all new tests fail?

Only PassManagerTest.FunctionPassCFGCheckerInvalidateAnalysis does fail.
The other two (PassManagerTest.FunctionPassCFGCheckerWrapped and PassManagerTest.FunctionPassCFGChecker) pass.

I would suggest to move new tests to separate test file. Mark it as expected to fail and do not change the value of -verify-cfg-preserved in this patch.
Then in a separate commit, turn the option be default and remove expected fail.
After that fix the issue and turn the option to on back.

If it is possible actually :)

I did not find a way to exclude except changing the line 'TEST_F(PassManagerTest, ...) to just a func definition' to skip this test. But this way the tests will not be run.
So, if you do not mind, I will land as is.

I do not mind.

kuhar accepted this revision.Nov 17 2020, 3:24 PM

LGTM

This revision was landed with ongoing or failed builds.Nov 17 2020, 7:04 PM
This revision was automatically updated to reflect the committed changes.
yrouban marked an inline comment as done.

Hi @yrouban, thank you for working on this! Unfortunately this breaks shared library builds (i.e. with BUILD_SHARED_LIBS set to ON). This is causing one of our buildbots to fail:

Admittedly, it's been failing in the past 24hrs for other reasons too :)

As the fix seemed trivial, I took the liberty of submitting a fixing patch without a review: https://github.com/llvm/llvm-project/commit/ccf500ce00c5768ae98248a8de90408488235aa7. Tested with BUILD_SHARED_LIBS and it builds fine. But I'm not that familiar with this codebase, so please comment/revert/update if I missed sth. Ta!

Hi @yrouban, thank you for working on this! Unfortunately this breaks shared library builds (i.e. with BUILD_SHARED_LIBS set to ON). This is causing one of our buildbots to fail:

Admittedly, it's been failing in the past 24hrs for other reasons too :)

As the fix seemed trivial, I took the liberty of submitting a fixing patch without a review: https://github.com/llvm/llvm-project/commit/ccf500ce00c5768ae98248a8de90408488235aa7. Tested with BUILD_SHARED_LIBS and it builds fine. But I'm not that familiar with this codebase, so please comment/revert/update if I missed sth. Ta!

Thanks. I believe you did all right. There were several unresolved symbols from SimplifyCFG reported as needed by PassManagerTest.cpp.