Page MenuHomePhabricator

[(new) Pass Manager] instantiate SimplifyCFG with the same options as the old PM
ClosedPublic

Authored by spatel on Oct 29 2017, 10:41 AM.

Details

Summary

The old PM sets the options of what used to be known as "latesimplifycfg" on the instantiation after the vectorizers have run, so that's what I'm proposing to do here.

FWIW, there's a later SimplifyCFGPass instantiation in both PMs where we do not set the "late" options. I'm not sure if that's intentional or not.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Oct 29 2017, 10:41 AM
This revision is now accepted and ready to land.Oct 29 2017, 11:03 AM
This revision was automatically updated to reflect the committed changes.
spatel reopened this revision.Nov 9 2017, 6:12 AM
spatel added a reviewer: dlj.

Reopening because this patch was reverted with:
https://reviews.llvm.org/rL317444

More details about the crash are in:
https://bugs.llvm.org/show_bug.cgi?id=35210

This revision is now accepted and ready to land.Nov 9 2017, 6:12 AM

D40035 proved that this patch was not the cause of the crash, so I will recommit.

This revision was automatically updated to reflect the committed changes.

We're seeing about 10% regressions in an internal benchmark as a result of this change. Still digging further, but wanted to send a heads-up.

Further updating this: Taking the compiler from rL318299 with the new llvm/Transforms/Scalar/SimplifyCFG.h and setting needCanonicalLoops to false fixes the issue; taking the old llvm/Transforms/Scalar/SimplifyCFG.h and setting needCanonicalLoops to true reintroduces the issue. The values of other flags seem to be unrelated to this regression.

With profiling we're particularly seeing slowdowns in the DecompressAllTags<SnappyArrayWriter> function.

Thanks for narrowing it down. I probably can't offer much assistance with the regression for the next few days, but let me provide some backstory for this change and cc some people who might have insight.

This patch is part of a sequence intended to solve:
https://bugs.llvm.org/show_bug.cgi?id=34603
...for both pass managers.

The 'NeedCanonicalLoop' portion in question was introduced/discussed in:
D35411

IMO, it was just an oversight that that change only affected the legacy pass manager. That was a carried limitation from the commit that introduced "-latesimplifycfg":
D30333