-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Create TargetDirKey to expose Stage's --target-dir to Config #2725
Conversation
TargetDirKey is a Field[String] that gets populated with the TargetDirectoryAnnotation during PreElaboration.
Just take a glance to this PR, It seems that adding |
|
||
case class WithTargetDir(dir: String) extends Config((site, here, up) => { | ||
case TargetDirKey => dir | ||
}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
My understanding is that I confess I don't understand how or whether |
@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. |
There was a problem hiding this 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.
FWIW, i think (in the short term) the 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. |
> 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. |
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.
TargetDirKey
is aField[String]
that gets populated with theTargetDirectoryAnnotation
during
PreElaboration
.Enables other
Config
Field
s 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 theStage
. AddingTargetDirKey
will allowchipyard.config.WithBootROM
to look for contents relative to the TargetDir. This will be helpful as we develop distributed elaboraton flows for Firesim in firesim#651Related issue:
Type of change: feature request
Impact: API addition (no impact on existing code)
Development Phase: proposal
Release Notes
Create a
TargetDirKey
thatPreElaboration
populates with value ofTargetDirAnnotation