Skip to content

Conversation

seldridge
Copy link
Member

This reverts a suggestion [1] I made on #8458. This changes
the (undocumented) ABI of FIRRTL layers to not include any directory
information in the `include directives used to enable layers. The
reason for this is that using the full paths (relative to the root output
directory) prevents rearranaging directories after compilation.

The downside to this is that users must include more +incdir command
line options to include all directories in a design. However, I think
this is reasonable.

I do think that we can roll this back to use the original behavior in the
future in a backwards-compatible way. I.e., making the change from not
including the full path to including the full path won't break any
downstream tools as these will just be including too many +incdirs.

This reverts a suggestion [[1]] I made on #8458.  This changes
the (undocumented) ABI of FIRRTL layers to _not_ include any directory
information in the `` `include `` directives used to enable layers.  The
reason for this is that using the full paths (relative to the root output
directory) prevents rearranaging directories after compilation.

The downside to this is that users must include more `+incdir` command
line options to include _all_ directories in a design.  However, I think
this is reasonable.

I do think that we can roll this back to use the original behavior in the
future in a backwards-compatible way.  I.e., making the change from not
including the full path to including the full path won't break any
downstream tools as these will just be including _too many_ `+incdir`s.

[1]: #8458 (review)

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge marked this pull request as ready for review May 10, 2025 00:52
@seldridge seldridge requested a review from darthscsi as a code owner May 10, 2025 00:52
@seldridge seldridge requested a review from rwy7 May 10, 2025 00:52
seldridge added a commit to chipsalliance/chisel that referenced this pull request May 12, 2025
Set `+incdir` for every primary source directory.  Previously, only the
root primary source directory would be set.

This change is done to allow for the FIRRTL ABI related to bind layers to
be changed to _not_ use an FIRRTL compiler output directory-relative path
for `` `include `` [[1]].  This is necessary because some flows are
rearranging the directory structure of Chisel-generated Verilog, which
includes layers, and paths relative to the root will break.

This is a compromise between complexity of the build system, ABI
simplicitly, and ease of directory manipulation.  This has the effect of
users with custom build flows will need to recognize that they need to set
the `+incdir` recursively based on all output directories that were
created.  Practically, this is easy to do.

[1]: llvm/circt#8474

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
seldridge added a commit to chipsalliance/chisel that referenced this pull request May 12, 2025
Set `+incdir` for every primary source directory.  Previously, only the
root primary source directory would be set.

This change is done to allow for the FIRRTL ABI related to bind layers to
be changed to _not_ use an FIRRTL compiler output directory-relative path
for `` `include `` [[1]].  This is necessary because some flows are
rearranging the directory structure of Chisel-generated Verilog, which
includes layers, and paths relative to the root will break.

This is a compromise between complexity of the build system, ABI
simplicitly, and ease of directory manipulation.  This has the effect of
users with custom build flows will need to recognize that they need to set
the `+incdir` recursively based on all output directories that were
created.  Practically, this is easy to do.

[1]: llvm/circt#8474

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge merged commit 025bb22 into main May 13, 2025
5 checks passed
@seldridge seldridge deleted the dev/seldridge/firrtl-drop-path-to-layer-includes branch May 13, 2025 01:05
unlsycn added a commit to chipsalliance/t1 that referenced this pull request May 27, 2025
ref: llvm/circt#8474

Signed-off-by: unlsycn <unlsycn@unlsycn.com>
unlsycn added a commit to chipsalliance/t1 that referenced this pull request May 27, 2025
ref: llvm/circt#8474

Signed-off-by: unlsycn <unlsycn@unlsycn.com>
unlsycn added a commit to chipsalliance/t1 that referenced this pull request May 27, 2025
ref: llvm/circt#8474

Signed-off-by: unlsycn <unlsycn@unlsycn.com>
sequencer pushed a commit to chipsalliance/t1 that referenced this pull request May 28, 2025
ref: llvm/circt#8474

Signed-off-by: unlsycn <unlsycn@unlsycn.com>
TaoBi22 pushed a commit to TaoBi22/circt that referenced this pull request Jul 17, 2025
This reverts a suggestion [[1]] I made on llvm#8458.  This changes
the (undocumented) ABI of FIRRTL layers to _not_ include any directory
information in the `` `include `` directives used to enable layers.  The
reason for this is that using the full paths (relative to the root output
directory) prevents rearranaging directories after compilation.

The downside to this is that users must include more `+incdir` command
line options to include _all_ directories in a design.  However, I think
this is reasonable.

I do think that we can roll this back to use the original behavior in the
future in a backwards-compatible way.  I.e., making the change from not
including the full path to including the full path won't break any
downstream tools as these will just be including _too many_ `+incdir`s.

[1]: llvm#8458 (review)

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
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.

3 participants