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

Operator like .part() that returns a non-overlapping chunk of signal #148

Closed
whitequark opened this issue Jul 14, 2019 · 1 comment
Closed
Labels
Milestone

Comments

@whitequark
Copy link
Contributor

The reason Value.part exists in Migen (I think; @sbourdeauducq could confirm) is that it lowers directly to Verilog's indexed part-select construct: x.part(s, N)x[s+:N]. Or at least was supposed to, because the semantics of these constructs don't match (m-labs/migen#168), and it's not valid on LHS anyway. In reality it lowers to something like N'(x >> s) on the RHS, and is legalized on the LHS.

We're stuck with this operator in nMigen due to backwards compatibility, but I think it is not very useful: it is essentially a bit-slip primitive, whereas most applications for indexed part-select call for a word-slip primitive, which would essentially be x.<oper>(s, N)x[s*N:+N].

It would be good to have a word-slip primitive as first class, because it is a relatively common operation that is awkward to do in nMigen. The obvious solution x.part(s*N, N) is bad for several reasons: it requires duplicating a part of an expression, which is prone to copy-paste errors, and it expands to inefficient code (multiplication followed by a shift) that may or may not be optimized away, and in my limited testing is not handled very well by toolchains. The correct solution Array(s[x*8:x*8+8] for x in range(len(s)//8)) is very tedious to write each time you need it, but expands to efficient logic.

Bikeshed: what should it be named? Value.chunk? I'm worried that, part being nondescriptive as it is, you'd never remember which of the part or chunk (or a similar new operator) one should use. Perhaps it would be wise to rename the operator to something more sensible and deprecate part, retaining it as a compatibility name only.

@whitequark whitequark added this to the 0.1 milestone Jul 14, 2019
@sbourdeauducq
Copy link
Member

sbourdeauducq commented Jul 15, 2019

We're stuck with this operator in nMigen due to backwards compatibility, but I think it is not very useful: it is essentially a bit-slip primitive

The patch adding this wasn't by me:
m-labs/migen@2d37c78

See discussion here:
m-labs/migen#123

I'm not sure how much code actually uses it (especially as it's a recent addition), so perhaps it can be removed/replaced by something better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants