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

[nmigen-issue-migration]

Issue by whitequark
Sunday Jul 14, 2019 at 03:04 GMT
Originally opened as m-labs/nmigen#148

---

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.

[nmigen-issue-migration]

Comment by sbourdeauducq
Monday Jul 15, 2019 at 03:41 GMT

---

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.