Page MenuHomePhabricator

[mlir][dataflow] (WIP) Overhaul analysis state management to support composability
Changes PlannedPublic

Authored by Mogball on Jun 29 2022, 5:25 PM.

Details

Reviewers
aartbik
Summary

This patch splits AnalysisState into two classes: AbstractElement and AbstractState.
Now, AbstractState purely contains "facts" about the IR, whereas AbstractElement
contains all the dependency management. Crucially, this distinction allows two
version of AbstractElement, one which contains a single state and one which
contains a "main" state and several substates for analyses that are statically known
to produce values for that state.

SingleStateElement behaves the same as AnalysisState, but MultiStateElement
combines states produced by different analyses to support composability, allocating
a substate for each analysis statically known to produce that state.

The main things I would like feedback on are:

  • the staticallyProvides function and whether there's a better way for analyses to indicate what they provide (it works but it's awkward to write)
  • the rather complex class hierarchy that exists now with AbstractState, AbstractElement, and its two main implementations (confusing API)
  • whether the update function is more ergonomic then manually calling propagateIfChanged

Depends on D128867

Diff Detail

Event Timeline

Mogball created this revision.Jun 29 2022, 5:25 PM
Herald added a project: Restricted Project. · View Herald Transcript
Mogball requested review of this revision.Jun 29 2022, 5:25 PM
Mogball planned changes to this revision.Jun 29 2022, 5:26 PM
Mogball retitled this revision from [mlir][dataflow] (WIP) Overhaul analysis state management to support composability This patch splits `AnalysisState` into two classes: `AbstractElement` and `AbstractState`. Now, `AbstractState` purely contains "facts" about the IR, whereas... to [mlir][dataflow] (WIP) Overhaul analysis state management to support composabilityThis patch splits `AnalysisState` into two classes: `AbstractElement` and `AbstractState`.Now, `AbstractState` purely contains "facts" about the IR, whereas....
Mogball edited the summary of this revision. (Show Details)
Mogball retitled this revision from [mlir][dataflow] (WIP) Overhaul analysis state management to support composabilityThis patch splits `AnalysisState` into two classes: `AbstractElement` and `AbstractState`.Now, `AbstractState` purely contains "facts" about the IR, whereas... to [mlir][dataflow] (WIP) Overhaul analysis state management to support composability.
Mogball edited the summary of this revision. (Show Details)
martong removed a subscriber: martong.Jun 30 2022, 9:12 AM

I think update() is better than manually calling propagateIfChanged() because it reduces the chance of misuse without requiring more code.

I haven't taken a deep look so I don't have opinion on staticallyProvides and AbstractElement yet.

Is it possible to create a patch that just contains update()?

mlir/include/mlir/Analysis/DataFlowFramework.h
310

Maybe return const StateT &?