Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create TargetDirKey to expose Stage's --target-dir to Config #2725

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

timsnyder-siv
Copy link
Contributor

TargetDirKey is a Field[String] that gets populated with the TargetDirectoryAnnotation
during PreElaboration.

Enables other Config Fields to refer to the Stage's TargetDir. Specifically, the current chipyard.config.WithBootROM depends on finding a file relative to the current-working directory of the Stage. Adding TargetDirKey will allow chipyard.config.WithBootROM to look for contents relative to the TargetDir. This will be helpful as we develop distributed elaboraton flows for Firesim in firesim#651

Related issue:

Type of change: feature request

Impact: API addition (no impact on existing code)

Development Phase: proposal

Release Notes

Create a TargetDirKey that PreElaboration populates with value of TargetDirAnnotation

TargetDirKey is a Field[String] that gets populated with the TargetDirectoryAnnotation
during PreElaboration.
@sequencer
Copy link
Member

Just take a glance to this PR, It seems that adding TargetDirAnnotation can alter TargetDirKey, while adding TargetDirKey won't change real targetDir, since no TargetDirAnnotation is added by PreElaboration.

@sequencer sequencer self-requested a review November 10, 2020 20:46

case class WithTargetDir(dir: String) extends Config((site, here, up) => {
case TargetDirKey => dir
})
Copy link
Member

Choose a reason for hiding this comment

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

I think including WithTargetDir as a public class is a bit confusing since the intention isn't to allow users to select a new target dir by directly manipulating this Field, but rather only to record what was done via the command line, right? To prevent confusion on this point I suggest dropping this class in favor of a more direct alteration below.

Copy link
Member

Choose a reason for hiding this comment

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

Or we can add TargetDirAnnotation(config(TargetDirKey)) at PreElaboration? then modification to config will change TargetDirAnnotation.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that seems it could be better to make them able to change together, but if they conflict with each other, which one wins? Config or command line?

I also don't understand where TargetDirAnnotation is being added right now. Is it problematic to apply/change it during PreElaboration based on the Config value?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I agree that seems it could be better to make them able to change together, but if they conflict with each other, which one wins? Config or command line?

Yes, that's a problem, I also encounter this at chiseltest refactoring, if conflict, I choose to throw this Error to user, but I don't think this is a good way.

Copy link
Contributor

@debs-sifive debs-sifive Nov 10, 2020

Choose a reason for hiding this comment

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

I also don't understand where TargetDirAnnotation is being added right now. Is it problematic to apply/change it during PreElaboration based on the Config value?

Not sure if I understand your question correctly, but I believe it's assigned with the -td flag in the make flow here: [whoops removed internal link].

Copy link
Member

@hcook hcook Nov 10, 2020

Choose a reason for hiding this comment

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

Right, my question was more: which phase is in charge of turning a -td flag into a Scala string, and does it happen before or after rocketchip.phases.PreElaboration. Below @timsifive said

TargetDirAnnotation is ensured to be on the design by Checks phase and it is the direct prereq of PreElaboration.

and I think what @sequencer is proposing is that rather than being a pre-req, it becomes something that happens / gets applied during PreElaboration instead, so that Config Field values can affect it. But I don't understand the potential consequences of making that change to have it only be defined later.

Copy link
Contributor

@debs-sifive debs-sifive Nov 10, 2020

Choose a reason for hiding this comment

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

which phase is in charge of turning a -td flag into a Scala string

I don't think there's any one phase that does this. Phases that need the target dir take in the TargetDirAnnotation and then convert it/use it as needed.

The conversion from flag to annotation is in FIRRTL: https://github.com/freechipsproject/firrtl/blob/bbcee06d406c3755c10d41a71d69a69eb7d6f321/src/main/scala/firrtl/options/StageAnnotations.scala#L84

@sequencer's proposal seems safe in the sense that nothing before PreElaboration (besides Checks) uses the target dir for anything.

@hcook
Copy link
Member

hcook commented Nov 10, 2020

My understanding is that TargetDirKey is meant to be a record of the targetDir selected via the command line; the user is not meant to manipulate TargetDirKey themselves in order to change the targetDir. I made a suggestion that might help clarify this point, assuming my understanding is correct.

I confess I don't understand how or whether TargetDirAnnotation itself is added by PreElaboration as part of which project.

@hcook hcook requested review from hcook and debs-sifive November 10, 2020 21:11
@timsnyder-siv
Copy link
Contributor Author

TargetDirAnnotation is ensured to be on the design by Checks phase and it is the direct prereq of PreElaboration.

@hcook , your understanding of what I'm trying to do is exactly correct. Just trying to get the CLI value into Config so that it can be used, not changed.

Copy link
Member

@hcook hcook left a comment

Choose a reason for hiding this comment

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

I'm going to approve this because I think the current functionality of recording the command-line set value is immediately useful. But if @sequencer would like to work on some way of combining a Config-based value with a CLI-based value I do think that could be a good follow-up PR.

@davidbiancolin
Copy link
Contributor

FWIW, i think (in the short term) the Key should be made package private (per Henry's initial suggestion) and the command line option should always win.

Indeed no phase is responsible for adding that annotation, the command line options are translated to annotations before any phase runs. See firrtl.options.Shell.
The definition of a CLI argument extends HasShellOptions and defines how it should be translated to an annotation. Here's where that happens for TargetDir..

@hcook hcook merged commit 0049f6e into chipsalliance:master Nov 11, 2020
@sequencer
Copy link
Member

sequencer commented Nov 12, 2020

> But if @sequencer would like to work on some way of combining a Config-based value with a CLI-based value I do think that could be a good follow-up PR.

I’ll give a new PR for this.

@timsnyder-siv timsnyder-siv deleted the create-targetdirkey branch November 12, 2020 20:07
timsnyder-siv added a commit to sifive/chipyard that referenced this pull request Dec 22, 2020
Uses a forward-merged version of the topic-branch at chipsalliance/rocket-chip#2767
until chipyard#742 is merged and can subsume this intermediate bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants