<whitequark[cis]>
i feel like `with m.If(self.i.valid & self.i.ready):` as a pattern is pervasive enough to warrant a method on `stream.Interface`
<whitequark[cis]>
something like with m.If(self.i.transfer()):
<whitequark[cis]>
this is motivated by finding a number of bugs where i took a shortcut of using e.g. with m.If(self.i.valid): on the count of self.i.ready.eq(1) nearby, but then the assumption was no longer valid after refactoring
<tpw_rules>
perhaps silly question, does this have the downside of requiring long input pauses after end tokens if the fifo is full?
peeps[zen] is now known as peepsalot
<tpw_rules>
perhaps to phrase it another way, under what situations does the encoder itself create backpressure other than backpressure at its output?
<tpw_rules>
it looks like there's a few cycles pause after each end token
<tpw_rules>
oh i see how it works now with the staging, very clever
<whitequark[cis]>
the encoder has a 100% memory cycle efficiency, which is the limiting factor (i don't want to need two write ports; this doesn't work out too well on ice40, although it should be fine on other FPGAs)
<whitequark[cis]>
at the beginning and end of a packet it has a 1 cycle of backpressure
<whitequark[cis]>
re: staging, thank you. it took a serious effort to get it to work this elegantly (and be this readable)
<whitequark[cis]>
i basically reuse the "empty" space in the FIFO as the lookahead buffer, and save one BRAM worth in the process
<tpw_rules>
yeah it took a bit of thinking to get that but i like it
<whitequark[cis]>
a naive implementation would have between 50% and 75% of BRAM space overhead, which i was not happy with
<tpw_rules>
i would personally like to see a couple line comment explaining the staging. also unsure of the significance of "ensure that all preceding data is flushed via the output stream", on first read that kind of implied it would wait before accepting more input tokens until the fifo was empty which led to my earlier question
<tpw_rules>
on the other hand amaranth is always a pleasure to read ;)
<whitequark[cis]>
the data that you put in doesn't get immediately shifted out even if it has no zero bytes inside it
<tpw_rules>
also i guess it slows to half speed if your input data is all zeros
<whitequark[cis]>
it doesn't
<whitequark[cis]>
that's the whole point of using such a complex algorithm
<whitequark[cis]>
the overhead for all-NUL bytes is the same as for all non-NUL bytes
<tpw_rules>
in terms of data output yes, but i mean in terms of bytes accepted from the input
<tpw_rules>
that is probably not a severe constraint in many cases, but it could occasionally be a problem
<whitequark[cis]>
oh
<whitequark[cis]>
oh crap.
<tpw_rules>
oops
<whitequark[cis]>
that wasn't intended to be the case at all, and it will be performance-limiting in important cases
<tpw_rules>
could you fix it by just duplicating all of the data logic into start
<whitequark[cis]>
i think this is fixable though since i don't actually need to stage a known byte after committing
<whitequark[cis]>
since i know that i will overwrite it later anyway
<tpw_rules>
do you need that start stage at all, i think it is always overwritten by the commit? then just reset staged to 1 instead of 0
<whitequark[cis]>
oh, yeah, it's doing all sorts of things I decided I can't afford :p
<whitequark[cis]>
in particular I excluded a length FIFO from the beginning, and then tried (and succeeded) in excluding a second BRAM at all
<whitequark[cis]>
I don't see mine performing worse since it should be cycle-optimal (output on every cycle, input on every cycle where it's not outputting a ff overhead byte)
<zyp[m]>
it passes my tests too (but I'm only testing for correctness in corner cases, not performance)
<whitequark[cis]>
note that I have a slightly different semantics for end
<whitequark[cis]>
it's mainly to allow creating an end-of-packet condition without pushing another data byte in, which is critical for my use case
<whitequark[cis]>
but is incompatible with the semantics of last
<zyp[m]>
yeah, I noticed when my tests failed after just hooking end to last :)
<zyp[m]>
it also got me thinking; maybe what my stream Packet shape needs is a semantics enum argument instead of the current has_first/has_last arguments
<whitequark[cis]>
we should have a meeting next Monday, 1700 UTC like usual
<vup>
is it intentional, that this: https://github.com/amaranth-lang/amaranth/blob/fd41201e20592f412b15fd33fd29a9eb36899c69/amaranth/hdl/_ir.py#L57 overwrites any previous origins that were recorded for a `Fragment`? With this, using for example `DomainRenamer`, you end up with a `Fragment` that has just `TransformedElaboratable` in its `origins`. Why should the old origins there not be prepended, so you get `Elaboratable` and the `Module` that got stuck into
<vup>
the `DomainRenamer`?
<whitequark[cis]>
that was intentional (I vaguely recall being aware of the overwriting behavior and deciding this semantic is the one I should pick) but probably poorly thought out
<whitequark[cis]>
vup: i'm open to changing that, moreso that it's an unstable API
<whitequark[cis]>
* unstable API (without a real spec)
<vup>
changing it (or having `TransformedElaboratable` save the original fragment) would allow anuejn and me to remove a bunch of hacky and annoying code from naps atleast.
<vup>
What currently defines what is a unstable or a stable API btw? Because I noticed, `Fragment` is importable from `amaranth`
<zyp[m]>
anything that's documented is stable, anything prefixed with _ is private and should be considered unstable, anything else is somewhere in between? :)
<whitequark[cis]>
ys
<whitequark[cis]>
* yes
<whitequark[cis]>
Fragment in particular is not stable despite fairly pervasive use, except for Fragment.get which is necessary for some patterns and we'll have to support it
<whitequark[cis]>
(basically, you can't introspect the insides of a fragment, we have and will rearrange them)
<whitequark[cis]>
* rearrange them with no warning or compatibility)
<whitequark[cis]>
<vup> "changing it (or having `..." <- `TransformedElaboratable` is also unstable... ish? I don't think we'll entirely remove it but it'll probably be eventually documented as an opaque object that forwards all accesses to an inner thing
<whitequark[cis]>
actually no, it'll have to be redesigned for scopes
<vup>
yeah I am fine with `TransformedElaboratable` / `Fragment.origins` being unstable for our use in naps. Using it (or changing how `Fragment.origins` gets overwritten) would facilitate removing a bunch of hacky code in naps, if `Fragment.origins` or `TransformedElaboratable` vanishes some day, so be it, we'll just add the hacky code back (or replace it with some stable amaranth API that might exists by then). But I have no argument from just amaranth's point
<vup>
of view for changing the behaviour of `Fragment.origins`, so I can understand if that might not be enough reasoning for changing `Fragment.origins`.
<whitequark[cis]>
I mean, I think the point you're making (DomainRenamer losing origin information) is compelling
<whitequark[cis]>
can you show me said hacky code?