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

Boring from parent module with probe doesn't get a necessary :#= assignment #3557

Closed
mwachs5 opened this issue Sep 27, 2023 · 2 comments · Fixed by #4083
Closed

Boring from parent module with probe doesn't get a necessary :#= assignment #3557

mwachs5 opened this issue Sep 27, 2023 · 2 comments · Fixed by #4083

Comments

@mwachs5
Copy link
Contributor

mwachs5 commented Sep 27, 2023

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

See the test in #3556. Even if fixing the issue reported there, the test will not pass, because as shown in the picture below, we implement the single black arrow not the dotted two-part arrow:
Screenshot 2023-09-26 at 4 50 21 PM

What is the current behavior?
Note the bottom arrow in the picture: when we are goign straight from a bidirectional thing, and then turning it into an Input, we dont have any part of the code that does the stripping of direction and then using a :#= operator to drive the stripped-direction input to the bidirectional "real thing".

When there is a layer of hierarchy in the middle:
Screenshot 2023-09-27 at 11 05 13 AM

we have to first create the probe, which handles the stripping and the define works without the #:=.

What is the expected behavior?

We need to not emit a connect from a bidirectional thing to a monodirectional thing.

This can be done by either emitting the individual fields (as in the :#= would handle) or by making a probe define and then read in the same layer of hierarchy.

The former is a little tricky to implement because we can't directly use the :#= operator in the boring APIs, we can only add commands to the secretCommands, and there is no direct translation from :#= to the commands we would want to add to secretCommands.

It might be simpler to just always make the probe, define, and read even if we are in the same layer of heirarchy.

Please tell us about your environment:

Other Information

What is the use case for changing the behavior?

Need to get code compiling. There is a possible workaround -- users can make the Wire(Output(...)) themself in the source of the tap.

@dtzSiFive
Copy link
Member

It might be simpler to just always make the probe, define, and read even if we are in the same layer of heirarchy.

This should be optimized away by CIRCT, if it's not LMK 👍 . It does seem like it may simplify things a bit.

In a sense a read(probe(X)) would become the implementation of a hypothetical asPassive(X) command, is that about right?

All good, but curious/interested: what's the reason/challenges preventing having :#= as a (sequence of) "secret command"?

@seldridge
Copy link
Member

All good, but curious/interested: what's the reason/challenges preventing having :#= as a (sequence of) "secret command"?

It's due to current implementation. This can be changed. The problem is that :#= eventually results in calls to := here. This will push commands to the module's normal command area. Instead, we have a problem where we need to push arbitrary commands. However, the module is currently only allowing you to create a secretConnection through a restricted API that only works with a LHS and RHS Data (which then pushes commands into the secret command array).

To be able to use :#= there needs to be a way to tell any connection what buffer to add commands to. If we're then starting to add arguments to :#= and friends related to what buffer to push to, I don't see the point in maintaining two separate buffers.

tl;dr: there are some infrastructure changes that have to happen.

dtzSiFive added a commit that referenced this issue May 23, 2024
Expected result is fully aligned, which is what happens
when reading from a probe.

When the source is already at the LCA (from parent)
there's no probe and the secret connections only support
a few commands.

For lack of a way to do, e.g., `:#=` here,
use `read(probe(x))` to get the fully aligned result
that's expected and bored down to the child.

This isn't ideal but fixes this using only the sorts of expressions
and commands that we've already committed to supporting.

Fixes #3557.
dtzSiFive added a commit that referenced this issue May 23, 2024
Expected result is fully aligned, which is what happens
when reading from a probe.

When the source is already at the LCA (from parent)
there's no probe and the secret connections only support
a few commands.

For lack of a way to do, e.g., `:#=` here,
use `read(probe(x))` to get the fully aligned result
that's expected and bored down to the child.

This isn't ideal but fixes this using only the sorts of expressions
and commands that we've already committed to supporting.

Fixes #3557.
jackkoenig pushed a commit that referenced this issue May 24, 2024
Expected result is fully aligned, which is what happens
when reading from a probe.

When the source is already at the LCA (from parent)
there's no probe and the secret connections only support
a few commands.

For lack of a way to do, e.g., `:#=` here,
use `read(probe(x))` to get the fully aligned result
that's expected and bored down to the child.

This isn't ideal but fixes this using only the sorts of expressions
and commands that we've already committed to supporting.

Fixes #3557.
mergify bot pushed a commit that referenced this issue May 24, 2024
Expected result is fully aligned, which is what happens
when reading from a probe.

When the source is already at the LCA (from parent)
there's no probe and the secret connections only support
a few commands.

For lack of a way to do, e.g., `:#=` here,
use `read(probe(x))` to get the fully aligned result
that's expected and bored down to the child.

This isn't ideal but fixes this using only the sorts of expressions
and commands that we've already committed to supporting.

Fixes #3557.

(cherry picked from commit 0dbedc3)

# Conflicts:
#	core/src/main/scala/chisel3/RawModule.scala
jackkoenig pushed a commit that referenced this issue May 24, 2024
Expected result is fully aligned, which is what happens
when reading from a probe.

When the source is already at the LCA (from parent)
there's no probe and the secret connections only support
a few commands.

For lack of a way to do, e.g., `:#=` here,
use `read(probe(x))` to get the fully aligned result
that's expected and bored down to the child.

This isn't ideal but fixes this using only the sorts of expressions
and commands that we've already committed to supporting.

Fixes #3557.

(cherry picked from commit 0dbedc3)

# Conflicts:
#	core/src/main/scala/chisel3/RawModule.scala
chiselbot pushed a commit that referenced this issue May 27, 2024
…4096)

* Fix boring tap of non-passive source from parent. (#4083)

Expected result is fully aligned, which is what happens
when reading from a probe.

When the source is already at the LCA (from parent)
there's no probe and the secret connections only support
a few commands.

For lack of a way to do, e.g., `:#=` here,
use `read(probe(x))` to get the fully aligned result
that's expected and bored down to the child.

This isn't ideal but fixes this using only the sorts of expressions
and commands that we've already committed to supporting.

Fixes #3557.

(cherry picked from commit 0dbedc3)

# Conflicts:
#	core/src/main/scala/chisel3/RawModule.scala

* Resolve backport conflicts

---------

Co-authored-by: Will Dietz <will.dietz@sifive.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.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 a pull request may close this issue.

3 participants