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

PropagatePresetAnnotations: remove false prerequisites #2323

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

ekiwi
Copy link
Contributor

@ekiwi ekiwi commented Aug 9, 2021

The SMT backend actually needs to run PropagatePresetAnnotations
(as will treadle at some point).
None of the Verilog specific passes were actually required!

Contributor Checklist

  • Did you specify the type of improvement?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?

Type of Improvement

  • bug fix (remove false dependencies)

API Impact

  • this will allow backends to require PropagatePresetAnnotations without pulling in Verilog specific dependencies.

Backend Code Generation Impact

  • ideally none, but could change pass order

Desired Merge Strategy

-squash

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

The SMT backend actually needs to run PropagatePresetAnnotations
(as will treadle at some point).
None of the Verilog specific passes were actually required!
@ekiwi ekiwi requested a review from seldridge August 9, 2021 20:15
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me.

@johnsbrew: Do you recall the original reason for having the prerequisites specified as such?

@ekiwi ekiwi added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Aug 9, 2021
@ekiwi ekiwi added this to the 1.5.0 milestone Aug 9, 2021
@mergify mergify bot merged commit 6e7982b into chipsalliance:master Aug 9, 2021
@johnsbrew
Copy link
Contributor

@johnsbrew: Do you recall the original reason for having the prerequisites specified as such?

@seldridge Sorry for the late answer. I was requested to have this transform to run as late as possible in the old sequential order, so I added as prerequisite a lot of other transforms as part of the migration to new dependency API which had just been merged at the time. None of them were actual prerequisites for the transform.

@seldridge
Copy link
Member

Thanks for the info, @johnsbrew. It looks like the change here should be fine. 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants