<whitequark[cis]>
<Wanda[cis]> "add_process functionality..." <- re interaction of `until` with `sample`: yes, I agree, IIRC it's in unresolved questions for that reason
<whitequark[cis]>
I would be much happier if until unconditionally did at least one tick
<whitequark[cis]>
re reduction of functionality of add_process
<whitequark[cis]>
it goes against the discussion on RFC 36: well the discussion was made without any consideration of the concurrency model or the implementation, so I think it's rather fair to ask people to reconsider it
<whitequark[cis]>
it's needed to implement phase shift blocks: addressed in the text
<whitequark[cis]>
s/it's/`delay`/
<whitequark[cis]>
get() needed to implement some flops: I think that will have to work without get() because I really don't want to have two types of get() now that I know we can (mostly at least) get by without that
<whitequark[cis]>
no sample() on edge(): there should just be sample() on edge().
<whitequark[cis]>
<Wanda[cis]> "you also say that sim.set..." <- oh, that... is an issue yeah, I was thinking of the DDR example, but it's very not implementable, and needs sample on edge
<whitequark[cis]>
<Wanda[cis]> "overall, I believe our processes..." <- that sounds desirable, considering that `add_process`es literal stated purpose is to be able to replace any RTL block
<whitequark[cis]>
<Wanda[cis]> "I think these examples just..." <- right yeah
<whitequark[cis]>
<Wanda[cis]> "return a list of tuples of..." <- that sounds like something you can, and should, just implement in python. the idea behind `repeat` is that you can offload cycle counting to C++ with CXXRTL. like `def testbench(): await tick().repeat(100000)`, to run your CPU core for 100kcyc. you definitely do NOT want a tuple returned from that.
<whitequark[cis]>
<galibert[m]> "I understood it as linear..." <- this sounds like SVA, and also a bad idea (the language gets incredibly complex if you want generality for that)
<Wanda[cis]>
uh, why would you want one class for it?
<Wanda[cis]>
and it's not value-dependent type, it's type-dependent type (memory[2:3] is Slice, memory[2] is Row)
<Wanda[cis]>
and agreed, it should be MemoryData.Row / MemoryData.Slice
<whitequark[cis]>
but... 2:3 is just slice and you can even pass that dynamically
<whitequark[cis]>
oh, I see
<whitequark[cis]>
yes, it's an overload
<whitequark[cis]>
nevermind
<whitequark[cis]>
I'm definitely happy to add MemoryData.Row (not sure where it should be done)
<whitequark[cis]>
I'm unclear on what the usefulness of MemoryData.Slice is
<Wanda[cis]>
you said it yourself — add first 1kiB of a huge memory to traces
<whitequark[cis]>
yes but you can do memory[i] for i in range(0x400)
<whitequark[cis]>
semantically, is there anything MemoryData.Slice can do that can't be done by passing around a list of MemoryData.Row?
<whitequark[cis]>
like do we get a performance benefit anywhere or something
<Wanda[cis]>
complexity improvement when you want to monitor a range of memory for changes
<whitequark[cis]>
I'm rather hesitant to add that precisely because of complexity
<Wanda[cis]>
er, I mean computational complexity
<whitequark[cis]>
yes, I understood
<whitequark[cis]>
with pysim for example, if you do this with the default aiter implementation, you get to do O(n) trigger add/removes per cycle
<whitequark[cis]>
and it's actually not entirely clear if you can do better without making pysim significantly more complex
<whitequark[cis]>
with cxxsim you end up in a deeper mess since, iirc, the current implementation does the comparisons on triggers in python
<whitequark[cis]>
so, yes, it does remove this algorithmic complexity from the client code (which we aren't sure anyone is going to write) and puts it into our language code (where we need to implement it before 0.5)
<Wanda[cis]>
oh? a trigger on a memory slice can be a single trigger with a filter (much like edge is), so the O(n) doesn't happen
<whitequark[cis]>
or in other words: I think that unless we can commit to making MemoryData.Slice efficient we need to not have it so that memory data indexing is obviously shown as an expensive operation
<whitequark[cis]>
Wanda[cis]: right, ok. but that's not a thing in cxxsim
<Wanda[cis]>
okay, but then
<Wanda[cis]>
is your objection only to Slice, or do you also object to having full MemoryData change-monitored as well?
<whitequark[cis]>
in cxxsim the latter would be equivalent to monitoring memory[:]
<whitequark[cis]>
so, yes, I think we should go for individual rows only in 0.5 and do slices (including full slices) when we have more resources (and desire) available to implement it in an efficient way
<whitequark[cis]>
probably post-cxxsim-merge (which will hopefully happen in 2024H1)
<Wanda[cis]>
mhm
<whitequark[cis]>
passing memory to traces can still add all of the memory rows, as purely a compatibility thing
<whitequark[cis]>
notwithstanding that it's often kind of a bad idea
<Wanda[cis]>
so, the whole thing of changed(MemoryData) came up because I wanted an async read port to be implementable via add_process
<whitequark[cis]>
wouldn't that just need to monitor the addressed row?
<Wanda[cis]>
it could
<whitequark[cis]>
that's more or less what cxxrtl itself does
<Wanda[cis]>
but there's a snag
<Wanda[cis]>
when the address changes, you need to read memory at the new address
<Wanda[cis]>
and you just removed sim.get
<whitequark[cis]>
right
<whitequark[cis]>
so, let me summarize
<whitequark[cis]>
we want to have an elegant way to implement anything you can do in RTL via `add_process`, and we desire it for completeness, rather than out of any concrete need or specific feature requests
<whitequark[cis]>
we have a concrete need to ship 0.5 and afterwards cxxsim, arising from feature requests and current performance issues
<whitequark[cis]>
`MemoryData.Slice` can be implemented with CXXRTL using commit observers [I just realized that] but cxxsim is *already* stalled on a number of edge case and less used features which has delayed it for literal years
<whitequark[cis]>
I think the most I can agree to, as an implementer, is that "we put MemoryData.Slice in as future work, we commit to adding it at an unspecified later time, and we actually do it after cxxsim is merged"
<whitequark[cis]>
CXXRTL commit observers give you the base address of the memory and the changed index, so yes, you could do a single trigger with both. I just don't want to deal with the headache of "CXXRTL now needs to expose a template-only method as a C ABI function with a Python callback" because of "we want it to be more elegant"
<whitequark[cis]>
or at least I don't want to deal with that on a deadline
<whitequark[cis]>
I'm sure it will have beautiful performance characteristics
<Wanda[cis]>
understood
<whitequark[cis]>
I think we should have add_process ≡ RTL as an explicit stated goal
<Wanda[cis]>
the cxxsim objection is very valid; I was thinking of pysim implementation where implementing it is essentially trivial
<whitequark[cis]>
I just don't think we can reach it just yet
<whitequark[cis]>
the interaction of cxxsim and memories is especially painful
<Wanda[cis]>
... I think we can if you simply don't remove sim.get
<whitequark[cis]>
(the current cxxsim branch is unsound because memory items change the base address after reset)
<whitequark[cis]>
Wanda[cis]: the fact that we have two behaviors of `yield value_like` is a massive hazard in the legacy API that I wanted to kill as long as I was thinking about adding `add_testbench`
<whitequark[cis]>
it's completely incomprehensible to anyone who hasn't thought about async simulators for a few months
<whitequark[cis]>
leaving it in would be quite possibly the biggest landmine in the language
<whitequark[cis]>
even worse than the precedence of `|` and `&` because we can fix this one with crimes but not that one
<Wanda[cis]>
... I agree, I was thinking of it too
<Wanda[cis]>
I figured we may just want to have distinct method names for process simulation contexts and testbench simulation contexts
<whitequark[cis]>
I initially agreed to it because I was too curr/next-brained
<whitequark[cis]>
and to me it was relatively obvious
<whitequark[cis]>
also I didn't think of a better solution
<whitequark[cis]>
Wanda[cis]: I'm not in principle opposed to that, but if the only reason to leave it in is `MemoryData.Slice` I think that's not justified
<Wanda[cis]>
and then we can just remove the weird testbench_helper decorator, since things will blow up on their own
<whitequark[cis]>
I already remove it in the amendment
<whitequark[cis]>
so I guess "we don't have to add it back"
<whitequark[cis]>
but I also like it that we nudge people towards using .sample() through lack of .get(), because .sample() is the right solution most of the time
<whitequark[cis]>
(nudge as in, "if you use .sample() your code works always and not just in testbenches")
<whitequark[cis]>
I'm still not totally sure if implementing FSMs via add_process that waits on different things is a good idea or not, but it might be?
<whitequark[cis]>
there's definitely things that are more clearly expressed as an infinite loop with await tick().sample(x); await tick().sample(y) in body
<whitequark[cis]>
throw in some until maybe
<Wanda[cis]>
hmmm.
<Wanda[cis]>
well
<whitequark[cis]>
I think it's pretty important that people prefer .sample() to .get() because of the example of get_stream I described earlier
<Wanda[cis]>
.sample means pre-committing to the set of signals you read
<Wanda[cis]>
.get_curr or whatever ugly offputting name we come up with lets you do it later
<whitequark[cis]>
ok, I think I don't want that in the initial implementation
<whitequark[cis]>
both because it's ugly (conceptually) and because I want to collect use cases first
<Wanda[cis]>
which is useful for when the set of shit you may want to read is large, but you only need a small data-depndent subset at any given time
<Wanda[cis]>
the obvious example being a large mux
<whitequark[cis]>
I actually was planning at one point to make add_process always pre-commit to the set of signals it'll read
<Wanda[cis]>
(which a memory async read port is)
<whitequark[cis]>
ok, so it's just an optimization?
<Wanda[cis]>
it's not "just" an optimization if a huge memory is involved
<whitequark[cis]>
why are you trying to implement a memory with add_process anyways? we already have memories
<whitequark[cis]>
(again: I have a deadline to ship this, I'm not really interested in "make things more elegant" right now. I'll be more interested in these when we have a working impl and a 0.5 release)
<whitequark[cis]>
* (again: I have a deadline to ship this, I'm not really interested in "make it more elegant by being able to implement more of RTL" right now. I'll be more interested in these when we have a working impl and a 0.5 release)
<whitequark[cis]>
(and that impl is something I will have to document and write a methodology for, in full, so I don't want cursed edge case shit that doesn't even have anyone who says they want it in more than a theoretical way)
<whitequark[cis]>
also if you just want a mux you can do await tick().sample(Array([...])[x]) or something
<whitequark[cis]>
since .sample() takes any value-like and we have several mux primitives in the language
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] b411e63 - Deploying to main from @ amaranth-lang/amaranth@0cb71f8c57d93fc1f390eb6074bb6e97d18af263 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 9f4b5ba - Deploying to main from @ amaranth-lang/amaranth@36fb9035e4d392054c76d4f218662206a6771bc9 🚀
<_whitenotifier-6>
[amaranth] github-merge-queue[bot] created branch gh-readonly-queue/main/pr-1233-36fb9035e4d392054c76d4f218662206a6771bc9 - https://github.com/amaranth-lang/amaranth
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 340ac26 - Deploying to main from @ amaranth-lang/amaranth@11f7b887ad382e71fa1ec98466fe9e6650aa7117 🚀
<whitequark[cis]>
(I don't care about all of the docstrings in _ast which are not already in the reference manual--I'll probably delete them and rewrite when I get to them
<whitequark[cis]>
* (I don't care about all of the docstrings in _ast which are not already in the reference manual--I'll probably delete them and rewrite when I get to them)
<whitequark[cis]>
ok, I suppose they're shown in help(Signal) which is all the more reason to delete them
<tpw_rules>
ok, i await your new review then
<whitequark[cis]>
well I do want error messages improved
<whitequark[cis]>
or do you want a specific wording from me?
<tpw_rules>
i can rearrange to swap "private Signal" for "signal without a name" or some similar. i more meant the logic review
<whitequark[cis]>
ah, yeah
<whitequark[cis]>
"signal with a private name" is probably the best
<whitequark[cis]>
clearly sets it apart from other signals, but does not suggest that there's something up with the signal, only with its name
<tpw_rules>
not s/private/empty/? but that does sound good
<whitequark[cis]>
the options for "what is a private name" aren't rich; "__x" would be on the mind of many Python programmers but that's easily discoverable in the doc, which will eventually be written
<whitequark[cis]>
tpw_rules: "empty" sounds a little ambiguous with `Signal()`
<whitequark[cis]>
meanwhile, "private Signal" may possibly mean "signal from within the hierarchy" or "self._x = Signal()" none of which are prohibited
<tpw_rules>
i assume you are still taking full control of the docs. i had thought about offering to contribute there but i recall you are opinionated
<whitequark[cis]>
correct. it's less about being opinionated and more because the docs are the language
<whitequark[cis]>
the implementation is simply a bunch of ASTs laying on the side of the road struggling to implement its shape
<whitequark[cis]>
so the offer you have boils down to "hi, let me make the language for you?"
<whitequark[cis]>
(to a lesser extent it's also because as I got better at writing docs, the bar for how good they should be got even higher, and now I'm kind of uncertain if anyone in the community at all, excluding those I've taught on how to do it, is going to meet it)
<tpw_rules>
sometimes i find it easier to start with something more extant than a blank page and then mold it in my image, but i understand your point
<whitequark[cis]>
oh, no, I find reviewing docs far more difficult than writing them, and coordinating with collaborators about docs is excruciating unless I can like, get you on a call for 6 hours straight
<whitequark[cis]>
I never seem to get writer's block for more than half a hour
<tpw_rules>
heh, maybe we could do that someday. i need the skill too... but i'll leave you to it, i do very much appreciate your work
<whitequark[cis]>
there were nontrivial changes to the MemoryData RFC and minor changes to Stream RFC, the latter mainly around the Future work section, as well as clarification of rules
<cr1901>
I don't understand the difference between Option (C2) and (C3) in 61. You say "option C2 requires adaptation", and yet option C3 ends by saying you have to put the FIFO signals into a PureInterface?
<cr1901>
"putting the FIFO signals into a PureInterface" sounds a lot like an adapter to me
adamgreig[m] has quit [Quit: Idle timeout reached: 172800s]
<cr1901>
(Actually, I'm having trouble the "serious drawback" in Option (C2). Does anyone have a playground snippet they could link or something showing why C2 won't work, but C3 will?
nyanotech has quit [Remote host closed the connection]
nyanotech has joined #amaranth-lang
<zyp[m]>
sure, hang on
<cr1901>
zyp[m]: Ack thanks, take your time :)
<whitequark[cis]>
<cr1901> "I don't understand the differenc..." <- I'm using "adaptation" in the AXI Stream document definition here (requires logic) rather than the Amaranth meaning (requires reshuffling)
<zyp[m]>
GenericInterconnect could be e.g. a FIFO that doesn't care at all about the payload content, just the width of it
<cr1901>
Okay, cool... tyvm
<cr1901>
In this particular example of C2, the Signature payload wouldn't need any special logic to adapt to the StructLayout payload in example of C3?
polysci_00232[m] has quit [Quit: Idle timeout reached: 172800s]