Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Enforce ordering on custom pass annotations #446

Closed
colinschmidt opened this issue Feb 17, 2017 · 4 comments · Fixed by #1123
Closed

Enforce ordering on custom pass annotations #446

colinschmidt opened this issue Feb 17, 2017 · 4 comments · Fixed by #1123
Assignees
Milestone

Comments

@colinschmidt
Copy link
Contributor

When directly calling a firrtl compiler with custom transforms the user has the ability to enforce an order between their passes. If annotations are used to add custom transforms there is no extant way to order them. This would be a very useful feature.

@shunshou
Copy link
Contributor

shunshou commented Feb 17, 2017

One thing I might add, and not sure if anyone's really tried -- for ResetInverter, i assume the invert(this) thing could also be run from some higher module that instantiates a Module with ResetInverter (or whatever it was called) trait. If that's the case, people might be able to order them to some degree... but when dealing with transforms done on the top level module, I'm not sure how one goes about doing that with Chisel Driver? I'm still kind of confused about how the (): => stuff works...

I guess it gets weird when you start mixing/matching module level transforms vs. circuit level transforms, etc. So there still needs to be some way to feed in critical ordering, should it be necessary.

@shunshou
Copy link
Contributor

As another example: I need my AddPads Transform to come before BlackBoxHelper, but I don't know if there's any way to guarantee that, given that both transforms should occur on LowForm.

@azidar azidar added this to the 1.0.0 milestone May 10, 2017
@azidar
Copy link
Contributor

azidar commented May 10, 2017

Current thoughts: list of passes required before/after running - topo sort, then insert lowering passes to fix form mismatches

@jackkoenig jackkoenig modified the milestones: 1.0.0, 1.1.0 Nov 29, 2017
@azidar azidar modified the milestones: 1.1.0, 1.2.0 Dec 15, 2017
@chick chick modified the milestones: 1.2.0, 1.3.0 Dec 17, 2018
seldridge added a commit that referenced this issue Feb 13, 2019
This adds a Dependency API (#948) to the firrtl.options package. This
adds two new methods to Phase: prerequisites and invalidates. The
former defines what Phases must have run before this Phase. The latter
is a function that can be used to determine if this Phase invalidates
another Phase. Additionally, this introduces a PhaseManager that
subclasses Phase that determines a sequence of Phases to perform a
requested lowering (from a given initial state ensure that some set of
Phases are executed).

This follows the original suggestion of @azidar in #446 for the
PhaseManager algorithm with one modification. The (DFS-based)
topological sort of dependencies is seeded using a topological sort of
the invalidations. This ensures that the number of repeated Phases is
kept to a minimum. (I am not sure if this is actually optimal,
however.) DiGraph is updated with a seeded topological sort which the
original topological sort method (linearize) now extends.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
seldridge added a commit that referenced this issue Feb 13, 2019
This adds the PhaseManager class which can be used to determine a
sequence of Phases that should be run given the new Dependency API. A
PhaseManager determines an ordering that results in some target Phases
being run (without invalidations) given an initial state (some other
set of Phases).

This follows the original suggestion of @azidar in #446 with
one (critical) modification to the algorithm: the (DFS-based)
topological sort of dependencies is seeded using a topological sort of
the invalidations (this uses the DiGraph.seededLinearize method). This
ensures that the number of repeated Phases is kept to a minimum. (I am
not sure if this is actually optimal, however.)

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
@seldridge seldridge mentioned this issue Feb 13, 2019
1 task
@seldridge
Copy link
Member

Current approach in #1123 is that you get two ways of defining dependencies:

  1. The custom Transforms can define prerequisites, dependents, and invalidates
  2. The order of the RunFirrtlTransformAnnotations is used in seeding the topological sort. This has the effect of transforms, with the same dependencies, running in the same order they're specified. This should match the current behavior.

Implementation-wise for (2) explicit dependencies are not added based on the order.

@seldridge seldridge self-assigned this Jul 19, 2019
@seldridge seldridge linked a pull request Mar 5, 2020 that will close this issue
4 tasks
@seldridge seldridge removed a link to a pull request Mar 5, 2020
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants