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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a1ebd8
Create TargetDirKey
timsnyder-siv Nov 10, 2020
1c5c9c0
Adjust addition of TargetDirKey to better reflect it's value coming f…
timsnyder-siv Nov 10, 2020
9033a1d
cleanup stupid TAB and unused Config import
timsnyder-siv Nov 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/main/scala/stage/phases/PreElaboration.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,18 @@ import chisel3.RawModule
import chisel3.stage.ChiselGeneratorAnnotation
import firrtl.AnnotationSeq
import firrtl.options.Viewer.view
import firrtl.options.{Dependency, Phase, PreservesAll}
import freechips.rocketchip.config.Parameters
import firrtl.options.{Dependency, Phase, PreservesAll, StageOptions}
import freechips.rocketchip.config.{Config, Field, Parameters}
import freechips.rocketchip.diplomacy._
import freechips.rocketchip.stage.RocketChipOptions
import freechips.rocketchip.util.HasRocketChipStageUtils

case object TargetDirKey extends Field[String](".")

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.

@debs-sifive @jackkoenig

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.


/** Constructs a generator function that returns a top module with given config parameters */
class PreElaboration extends Phase with PreservesAll[Phase] with HasRocketChipStageUtils {

Expand All @@ -20,10 +26,11 @@ class PreElaboration extends Phase with PreservesAll[Phase] with HasRocketChipSt

override def transform(annotations: AnnotationSeq): AnnotationSeq = {

val stageOpts = view[StageOptions](annotations)
val rOpts = view[RocketChipOptions](annotations)
val topMod = rOpts.topModule.get

val config = getConfig(rOpts.configNames.get)
val config = getConfig(rOpts.configNames.get) ++ WithTargetDir(stageOpts.targetDir)
hcook marked this conversation as resolved.
Show resolved Hide resolved

val gen = () =>
topMod
Expand Down