Page MenuHomePhabricator

[Inliner] Handle multiple `run` invocation of `ModuleInlinerWrapperPass` (NFC).
Needs ReviewPublic

Authored by michele.scandale on Aug 1 2022, 4:53 PM.

Details

Reviewers
mtrofin
aeubanks
Summary

The current implementation ModuleInlinerWrapperPass uses three pass
managers. In the run function two of the three pass manager are
always moved into the third one. This lead to undefined behavior (e.g.
crash) if the ModuleInlinerWrapperPass::run is executed more than
once on the same instance of the pass because additional run perform
again the move operations on object whose state is unspecified.
This commit addresses the issue by ensuring that the move operations are
performed only on the first invocation of ModuleInlinerWrapperPass::run.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 4:53 PM
michele.scandale requested review of this revision.Aug 1 2022, 4:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 4:53 PM

asking to learn - what's the motivation for running ::run more than once / why not have different instances of ModuleInlinerWrapperPass?

asking to learn - what's the motivation for running ::run more than once / why not have different instances of ModuleInlinerWrapperPass?

The use case I'm looking into is a system processing several modules with a custom optimization pipeline where I'd like to pay the setup cost of building the pass manager only once rather than for each module being processed.
This is something that used to work (modulo bugs once in a while) with the legacy pass manager. I'm trying this with the new pass manager infrastructure, and found few issues while experimenting (the issue addressed here, D130954 and D130955).

asking to learn - what's the motivation for running ::run more than once / why not have different instances of ModuleInlinerWrapperPass?

The use case I'm looking into is a system processing several modules with a custom optimization pipeline where I'd like to pay the setup cost of building the pass manager only once rather than for each module being processed.
This is something that used to work (modulo bugs once in a while) with the legacy pass manager. I'm trying this with the new pass manager infrastructure, and found few issues while experimenting (the issue addressed here, D130954 and D130955).

Wondering, is this something that's intentionally supported though?

Setting that question aside, maybe it'd make more sense we moved the finalization stuff in a ModuleInlinerWrapperPass::finish() or something like that, and then we just assert(Finalized) in ::run; that would address a current oddity with the wrapper, in that the pass composition is somewhat deferred until the run.

wdyt?

Wondering, is this something that's intentionally supported though?

Based on the discussion in D130954, it seems that currently there is no intention to support the use case I was experimenting with, therefore this change is not required.
From the initial performance analysis rebuilding the pass manager is much cheaper with the new pass manager than the legacy one, and for now I can use such alternative approach for my use case.

Setting that question aside, maybe it'd make more sense we moved the finalization stuff in a ModuleInlinerWrapperPass::finish() or something like that, and then we just assert(Finalized) in ::run; that would address a current oddity with the wrapper, in that the pass composition is somewhat deferred until the run.

wdyt?

Clients of ModuleInlinerWrapperPass would need to call finish before using it. I don't know if that acceptable or not as there is no finish for regular pass managers.
Perhaps it would be better to find a way to avoid the pass managers moves inside run. It seems that explicitly allocating the PassModel for a CGSCCPassManager would allow to keep a pointer to the CGSCCPassManager that would be returned via getPM, and still be able to prepare the module pass manager with the ModuleToPostOrderCGSCCPassAdaptor directly in the constructor of ModuleInlinerWrapperPass.

Wondering, is this something that's intentionally supported though?

Based on the discussion in D130954, it seems that currently there is no intention to support the use case I was experimenting with, therefore this change is not required.
From the initial performance analysis rebuilding the pass manager is much cheaper with the new pass manager than the legacy one, and for now I can use such alternative approach for my use case.

Setting that question aside, maybe it'd make more sense we moved the finalization stuff in a ModuleInlinerWrapperPass::finish() or something like that, and then we just assert(Finalized) in ::run; that would address a current oddity with the wrapper, in that the pass composition is somewhat deferred until the run.

wdyt?

Clients of ModuleInlinerWrapperPass would need to call finish before using it. I don't know if that acceptable or not as there is no finish for regular pass managers.
Perhaps it would be better to find a way to avoid the pass managers moves inside run. It seems that explicitly allocating the PassModel for a CGSCCPassManager would allow to keep a pointer to the CGSCCPassManager that would be returned via getPM, and still be able to prepare the module pass manager with the ModuleToPostOrderCGSCCPassAdaptor directly in the constructor of ModuleInlinerWrapperPass.

Right, so then the lines 1160-1166 would be the responsibility of the user, and the main responsibility of the ModuleInlinerWrapperPass is to set up / clear out correctly the advisor. SGTM.