[llvm-dev] new pass manager version of opt vs. legacy pass manager version

[Snider, Todd via llvm-dev]

Hello All,

 

My development group has been maintaining a downstream version of the monorepo that stays in sync with the upstream “main” branch, but we are still using the legacy pass manager in our local copy of the monorepo.

 

We’ve recently encountered a few instances of lit tests that are failing when run with the legacy pass manager version of opt, but pass when run with the new pass manager version of opt.

 

The situation raises a couple of questions:

• Is the legacy pass manager behavior being adequately tested by the buildbots?
• What expectation should there be that legacy pass manager behavior will be maintained in light of changes made to code that affects both the new pass manager version and the legacy pass manager version of opt?

 

I suspect that my group is not the only ones trying to stay in sync with the upstream LLVM main branch and keep using the legacy pass manager, and I anticipate the only long-term remedy for our situation is to move to using the new pass manager as soon as we can.

 

Thoughts?

 

Regards.

 

Todd Snider

Texas Instruments Incorporated

[Madhur Amilkanthwar via llvm-dev]

On Thu, Apr 22, 2021 at 9:03 PM Snider, Todd via llvm-dev <llvm...@lists.llvm.org> wrote:

Hello All,

 

My development group has been maintaining a downstream version of the monorepo that stays in sync with the upstream “main” branch, but we are still using the legacy pass manager in our local copy of the monorepo.

 

We’ve recently encountered a few instances of lit tests that are failing when run with the legacy pass manager version of opt, but pass when run with the new pass manager version of opt.

I am curious, why would the switch to the new pass manager make them fail as those tests must have been well running with the old pass manager. Are these tests present in the trunk?

The situation raises a couple of questions:

• Is the legacy pass manager behavior being adequately tested by the buildbots?
• What expectation should there be that legacy pass manager behavior will be maintained in light of changes made to code that affects both the new pass manager version and the legacy pass manager version of opt?

 

I suspect that my group is not the only ones trying to stay in sync with the upstream LLVM main branch and keep using the legacy pass manager, and I anticipate the only long-term remedy for our situation is to move to using the new pass manager as soon as we can.

 

Thoughts?

IMHO, switch to the new pass manager may not happen overnight for many orgs. I believe the new pass manager has a different pass ordering and invalidation mechanism as compared to the old one so there could be impact on compile-time and application runtime performance.

Regards.

 

Todd Snider

Texas Instruments Incorporated

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

--

Disclaimer: Views, concerns, thoughts, questions, ideas expressed in this mail are of my own and my employer has no take in it. 

Thank You.
Madhur D. Amilkanthwar

[David Blaikie via llvm-dev]

There certainly have been cases where we've accepted patches to
maintain non-default configurations passing tests (for instance I
think Sony changed the language version default of their clang fork
compared to upstream - and contributed a bunch of patches to clang
tests so they pass no matter which language version is the default (in
cases where it wasn't material/significant to the test in question))

So I could imagine it might be reasonable to accept patches that
update tests that aren't intended to test one pass manager or another
(but became over-constrained to the default NPM behavior) so they pass
with either enabled (if this is important to you, though, I'd strongly
advise setting up a buildbot to keep track of violations of this
invariant (though if it's a really uncommon config, which it seems it
is, probably best to not send fail-email to commiters, instead only to
your team who can triage and then submit a patch (directly or for
review, as needed) to generalize the test so it passes in all configs
(which, in some cases, might mean adding an explicit "use the new pass
manager" flag to the test))

I don't actually know if there's a CMake config to change the default
here - if there is, then it seems pretty reasonable to me to support
building/running the tests (& expecting them to pass) in that
configuration. If there isn't, then it's a bit more debateable.

[Johannes Doerfert via llvm-dev]

On 4/22/21 11:57 AM, David Blaikie via llvm-dev wrote:
> There certainly have been cases where we've accepted patches to
> maintain non-default configurations passing tests (for instance I
> think Sony changed the language version default of their clang fork
> compared to upstream - and contributed a bunch of patches to clang
> tests so they pass no matter which language version is the default (in
> cases where it wasn't material/significant to the test in question))
>
> So I could imagine it might be reasonable to accept patches that
> update tests that aren't intended to test one pass manager or another
> (but became over-constrained to the default NPM behavior) so they pass
> with either enabled (if this is important to you, though, I'd strongly
> advise setting up a buildbot to keep track of violations of this
> invariant (though if it's a really uncommon config, which it seems it
> is, probably best to not send fail-email to commiters, instead only to
> your team who can triage and then submit a patch (directly or for
> review, as needed) to generalize the test so it passes in all configs
> (which, in some cases, might mean adding an explicit "use the new pass
> manager" flag to the test))
>
> I don't actually know if there's a CMake config to change the default
> here - if there is, then it seems pretty reasonable to me to support
> building/running the tests (& expecting them to pass) in that
> configuration. If there isn't, then it's a bit more debateable.

I'd hope we move towards removing the old-PM completely in upstream.
Thus, I don't really think we should require all tests to pass with
the old-PM, e.g., new passes should be allowed that run only with the
new-PM. If this is a reasonable way forward, let's add an lit XFAIL
for the old-PM and allow such XFAILs to be upstreamed for easier
maintenance.

~ Johannes

[David Blaikie via llvm-dev]

Oh, yeah, I certainly wouldn't advocate for limiting ourselves in that way.

> If this is a reasonable way forward, let's add an lit XFAIL
> for the old-PM and allow such XFAILs to be upstreamed for easier
> maintenance.

Fair - I wouldn't personally mind if a test is low-cost to generalize
over both pass managers. But also don't mind if the rule is we don't
do that and only add XFAILs.

[Min-Yih Hsu via llvm-dev]

On Thu, Apr 22, 2021 at 8:34 AM Snider, Todd via llvm-dev <llvm...@lists.llvm.org> wrote:

Hello All,

 

My development group has been maintaining a downstream version of the monorepo that stays in sync with the upstream “main” branch, but we are still using the legacy pass manager in our local copy of the monorepo.

 

We’ve recently encountered a few instances of lit tests that are failing when run with the legacy pass manager version of opt, but pass when run with the new pass manager version of opt.

I don't think you're talking about upstream tests right? Did you rewrite some of the new-PM-only tests into old PM syntax?

 

 

The situation raises a couple of questions:

• Is the legacy pass manager behavior being adequately tested by the buildbots?
• What expectation should there be that legacy pass manager behavior will be maintained in light of changes made to code that affects both the new pass manager version and the legacy pass manager version of opt?

 

AFAIK many Passes share the same code base between old and new PM (except few like AlwaysInliner who has completely different behaviors between old / new PM), so any changes to their underlying implementation should reflect in both PMs. The biggest differences between old / new PM is probably their Pass ordering in the pipeline.

 

I suspect that my group is not the only ones trying to stay in sync with the upstream LLVM main branch and keep using the legacy pass manager, and I anticipate the only long-term remedy for our situation is to move to using the new pass manager as soon as we can.

 

Well, don't forget we're still using old PM to do the code generation (wink) so old PM _by itself_ won't go away any time soon. But I think _optimization_ Passes written in legacy syntax might be removed sooner.

-Min

 

Thoughts?

 

Regards.

 

Todd Snider

Texas Instruments Incorporated

_______________________________________________

LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

--

Min-Yih Hsu

Ph.D Student in ICS Department, University of California, Irvine (UCI).

[Philip Reames via llvm-dev]

I ran across a case like this recently.  In that particular case, it was an IPO related test and the root issue was a difference in how the pass managers handled declarations in CGSCC passes.  (There was a discussion on llvm-dev on the topic if you're interested.)

The practical takeaway is that certain tests needed -enable-new-pm explicitly set or disabled.  With the options parsing for (e.g. -function-attrs) implicitly selecting the currently enabled (in build config) pass manager, some tests need to be explicitly pinned to one. 

I would advocate for allowing -enable-new-pm=1 to be added to tests where relevant, and making no systematic attempt to keep all tests running on the old pm.  As mentioned already downthread, there will be an increasing class of behavior not supported on the new pm.

I also think we should explicitly set of deprecation timeline for the old pm in the main pipeline, but that's a separate discussion.  I can't wait until the day we rename LegacyPM to CodeGenPM.  :)

Philip

[Arthur Eubanks via llvm-dev]

A small nit, please use `opt -passes=foo` rather than `opt -foo -enable-new-pm` to run tests against the new PM. `-enable-new-pm` basically attempts to rewrite `opt -foo` into `opt -passes=foo`, and the new PM custom pass pipeline creation is designed around parsing the `-passes=`. At some point `-enable-new-pm` should go away and all tests should use `-passes=`.

[Johannes Doerfert via llvm-dev]

On 4/22/21 12:22 PM, Philip Reames via llvm-dev wrote:
> I ran across a case like this recently.  In that particular case, it
> was an IPO related test and the root issue was a difference in how the
> pass managers handled declarations in CGSCC passes. (There was a
> discussion on llvm-dev on the topic if you're interested.)
>
> The practical takeaway is that certain tests needed -enable-new-pm
> explicitly set or disabled.  With the options parsing for (e.g.
> -function-attrs) implicitly selecting the currently enabled (in build
> config) pass manager, some tests need to be explicitly pinned to one.
>
> I would advocate for allowing -enable-new-pm=1 to be added to tests
> where relevant, and making no systematic attempt to keep all tests
> running on the old pm.  As mentioned already downthread, there will be
> an increasing class of behavior not supported on the new pm.
>
> I also think we should explicitly set of deprecation timeline for the
> old pm in the main pipeline, but that's a separate discussion.  I
> can't wait until the day we rename LegacyPM to CodeGenPM.  :)

+1 for these ideas, after addressing the comment by Arthur. This is
better than XFAIL: old-PM for sure.
We can add `-passes=` to those tests to force the new-PM and go on
without requiring feature parity.

I also would love a deprecation timeline.

~ Johannes

>
> Philip
>
> On 4/22/21 8:33 AM, Snider, Todd via llvm-dev wrote:
>>
>> Hello All,
>>
>> My development group has been maintaining a downstream version of the
>> monorepo that stays in sync with the upstream “main” branch, but we
>> are still using the legacy pass manager in our local copy of the
>> monorepo.
>>
>> We’ve recently encountered a few instances of lit tests that are
>> failing when run with the legacy pass manager version of opt, but
>> pass when run with the new pass manager version of opt.
>>
>> The situation raises a couple of questions:
>>

>>   * Is the legacy pass manager behavior being adequately tested by the
>>     buildbots?
>>   * What expectation should there be that legacy pass manager behavior