<cr1901>
There is a reasonable chance I won't be here for the meeting tomorrow, so I'll say "merge if no objections are raised" for the RFCs and leave my questions/comments here:
<cr1901>
#59: "This has the slight disadvantage that the clock generator module...." <-- is it in principle possible to have a future RFC for clock generators that restores the "generate clocks based on platform" behavior, without a full-blown "allow propagation up" behavior?
<cr1901>
#61: I think the stream signature is fine for MVP. Substreams (TID/TDEST) are something I want for efbutils; when substreams have been brought up, the idea seems to be "put it in the payload". 1/2
<whitequark[cis]>
re 59: you could always pass parameters to the clock generator from wherever you instantiate it in elaborate
<whitequark[cis]>
re 61: those will definitely go into payload
<cr1901>
This works, but my mental model is "src/dst aren't part of a payload; they're describing how data gets from one place to another". This muddles the difference between structured data and interfaces for me
<whitequark[cis]>
everything that's not ready and valid is part of payload, though
<whitequark[cis]>
just like the TCP and IP headers are part of the Ethernet payload
<cr1901>
Something I _already_ have problems with when deciding when something should be an interface or structured data.
<whitequark[cis]>
(imagine putting TCP in your TDEST/TID streams. TCP would still describe "how data gets from one place to another" but it'd definitely be a part of the payload of the stream either way)
<tpw_rules>
i think the wrapping of the hypothetical transformer system should emphasize that further
<whitequark[cis]>
rule of thumb: "can you imagine stuffing group of signals into a FIFO wholesale? if yes it's probably lib.data. if no it's probably lib.wiring"
<tpw_rules>
cr1901: i mitigate that problem by not knowing anything about lib.data
<tpw_rules>
:P
<tpw_rules>
(fifos do not support arbitrary shapes yet do they?)
<cr1901>
Yes, according to your mental model that's fine. I find it jarring. And with that rule of thumb, I don't know whether I'd but the group into a FIFO wholesale (I might want different FIFOs per src/dst)
<cr1901>
Yea I understand I'm not getting interface signals for substreams. I'm still going to say something :P
<cr1901>
I hope my point-of-view is at least reasonable
<cr1901>
tpw_rules: Too late. I'm stuck in the "valley of I'm starting to know that I know nothing"
<whitequark[cis]>
in isolation it is; it's where it clashes with that of others where it creates unreasonability (this is by no means directed personally at you; it's a general observation)
<whitequark[cis]>
if you start putting signals on the toplevel of the stream it rapidly becomes impossible to either satisfy everyone or have an ecosystem of reliable interconnect, much less boht
<whitequark[cis]>
s/boht/both/
<whitequark[cis]>
if streams weren't so generic I'd have no issue having TID/TDEST on the toplevel (like if I was just reading your code)
<whitequark[cis]>
yes, it creates a bunch of tradeoffs for interconnect, but so long as it's just one person applying one set of rules consistently in one codebase (or even a few like minded people), it's basically tractable
<whitequark[cis]>
however the moment you open things up for an arbitrary set of people with divergent goals that starts to fail, in a spectacular way
<whitequark[cis]>
re rule of thumb: that was meant for the case where all of your signal directions is the same for a bunch of stuff in the interface
<whitequark[cis]>
usually it's not very ambiguous because you have a bunch of stuff going different directions (like how it is in AXI for example)
<cr1901>
Oh, yea, I've been meaning to apply that rule-of-thumb to Sentinel
<cr1901>
I finally got _all_ the Components laid out, and I realized: "Wow, some of the Signatures suck"
<whitequark[cis]>
it's not a fundamentally different problem than, say, "do I pass a struct to this C function or do I pass a bunch of arguments?"
<whitequark[cis]>
except that defining structs in C is a heavyweight operation and defining structs in Amaranth is not
<cr1901>
turning some members of those interfaces into structured data will make me happier
<cr1901>
(just haven't done it yet)
<whitequark[cis]>
anyway, with the proposed "stream combinators" you'd have something like stream.Signature(my_stream.Routing(PixelRGBA))
<cr1901>
this is by no means directed personally at you <-- ack. I refrained from the convo where everyone overloaded you a few days ago, but my disposition was still "merge, it's fine" FWIW
<cr1901>
yea that would probably work/I'd be happy with that
<whitequark[cis]>
oh, the streams thing isn't a personal interaction I had, it's an observation at a distance
<whitequark[cis]>
like, observation made from reading code and studying existing designs
<whitequark[cis]>
clearly a lot of people have conflicting code and clearly reconciling that doesn't always work, and probably doesn't work more often than does (see: LiteX streams)
<cr1901>
61 is fine as an agreeable base IMO. My comments about substreams are not meant to block that
<cr1901>
And substreams and last are the only extra features from AXI-Stream I really like (the position/null byte stuff is a lot of wtf-ery)
lf_ has joined #amaranth-lang
lf has quit [Ping timeout: 272 seconds]
<whitequark[cis]>
I think that's meant to be able to upconvert x-wide streams with unaligned data up to n*x-wide streams
<whitequark[cis]>
and I think it's a bit internally-designed-by-multiple-teams thing?
<cr1901>
I would have to consult the spec, but all that sounds right; the stream library I used in Verilog only supported one but not the other lol
<whitequark[cis]>
it's one of the several bits of AXI-Stream which seemed completely inapplicable to Amaranth
<cr1901>
No complaints from me there :P
<cr1901>
>do I pass a struct to this C function or do I pass a bunch of arguments <-- okay, this is an interesting way of looking at it, and confirms my personal observations that _in certain cases_, it's not always clear-cut whether to put a Signal into an interface or structured data.
<whitequark[cis]>
oh, it's absolutely not "clear-cut at all times"
<whitequark[cis]>
hence "rule of thumb": a broad idea that can serve as a basic test
* cr1901
nods
<whitequark[cis]>
I haven't really thought about this much during the development of lib.data and lib.wiring because they're both isomorphic to, well, function arguments and data structures
<whitequark[cis]>
so I figured that anyone who knows how to use those will intuitively grasp how to use these two
<cr1901>
No, I didn't not make the connection without writing code I'm afraid :P
<whitequark[cis]>
nope, that's preferred if anything
<whitequark[cis]>
at least if there's like, two places you might want to put the signature, and you can't or don't want to put it into a common python file
<cr1901>
It's not compatible with the annotation method of creating signatures (which doesn't give you an opportunity to flip, if you're defining a Component meant to be a responder). And I use both annotations and "make the sig in the constructor" (_incidentally_, I might add) in Sentinel
<cr1901>
I'm thinking of completely removing the annotation versions just to be consistent.
<whitequark[cis]>
yeah the annotation versions are a bit of an odd corner of the language
<whitequark[cis]>
I don't see any issue with choosing to ban them as a matter of style
<cr1901>
Yea, I'll probably ban that style then, just to make it so all signatures are from initiator POV (even if I only instantiate them in responders). Hopefully, ppl using annotations have the courtesy to tell me whether a Component is initiator or responder in docs :P
<whitequark[cis]>
yeah
<cr1901>
>you could always pass parameters to the clock generator from wherever you instantiate it in elaborate <-- I usually instantiate submodules in __init__, but I don't have a problem w/ clock generator being special/requiring __init__ being called in elaborate of a parent module.
<whitequark[cis]>
it kinda goes both ways
<whitequark[cis]>
for example lib.memory.Memory can only be called in elaborate
<whitequark[cis]>
and various IO buffers will usually be called there
<Wanda[cis]>
<whitequark[cis]> "for example lib.memory.Memory..." <- I don't think that's true?
<Wanda[cis]>
if you create the read ports and write ports right afterwards, there's nothing particularly wrong with it, even
<cr1901>
I also don't see how 62 will cause my linked code to stop working
<cr1901>
It's just that the RFC recommends putting some parts in elaborate that I'd elect to put in __init__
<Wanda[cis]>
the problems start when you call Memory() in __init__ and *_port() in elaborate
<whitequark[cis]>
Wanda[cis]: oh yeah you're right
<Wanda[cis]>
actually
<cr1901>
Wanda[cis]: Is this with or without 62?
<Wanda[cis]>
there's an obvious diagnostic we should add, I think
<cr1901>
Because my linked code _works_ right now
<Wanda[cis]>
cr1901: both
<whitequark[cis]>
cr1901: yeah sorry for confusion. however, your code is kind of already broken
<whitequark[cis]>
in that if you elaborate multiple times it'll keep adding read ports
<whitequark[cis]>
(but I think you already know that)
<Wanda[cis]>
Wanda[cis]: the diagnostic is: set frozen on `Memory` instance once it gets elaborated, raise an exception if you call `*_port` on it afterwards
<whitequark[cis]>
Wanda: yes good absolutely
<whitequark[cis]>
I can't believe I missed that
<Wanda[cis]>
we probably also want to freeze init after this?
<Wanda[cis]>
or, post-RFC 62, data.init
<cr1901>
Why would it elaborate multiple times? If I had multiple UCodeROMs they would be separate objects
<cr1901>
so the read_ports created would be to different memories (with the same content)
<whitequark[cis]>
like if you called Simulator(dut) on it multiple times?
<cr1901>
oh right, I do do that
<Wanda[cis]>
minor fix to RFC 62: changed MemoryInstance constructor to take MemoryData
<cr1901>
Wanda[cis]: Will the diagnostic detect my broken code? B/c I do the Memory in __init__, create read_port() in elaborate dance for Sentinel memories
<Wanda[cis]>
it will, if you use the design twice
<Wanda[cis]>
(it doesn't really have a way to detect it otherwise)
<whitequark[cis]>
cr1901: you also won't need to create Memory in `__init__` anymore after the MemoryData RFC
<cr1901>
Is there any easy way to tell whether it's safe to instantiate components in __init__ or not? Are (hypothetical) ClockGenerator and Memory (after the MemoryData RFC) the only ones that aren't safe?
<whitequark[cis]>
so Memory will be safe (as in, it will complain loudly if you try to misuse it) after the fix Wanda is talking about
<whitequark[cis]>
ClockGenerator isn't unsafe, it's just ... impossible? as in you don't have the information before elaborate
<cr1901>
right, have to pass the platform info in somehow
<cr1901>
MemoryData will be good b/c it decouples init from Memory instantiation, but I don't remember if that's actually why I instantiated Memory in __init__. It's quite possible I wasn't thinking hard about it.
<cr1901>
Anyways, no problems- I'll fix my code/test the new diagnostic. And just to recap- if I'm not here tomorrow, merge on everything if no other objections brought up.
Degi_ has joined #amaranth-lang
Degi has quit [Ping timeout: 260 seconds]
Degi_ is now known as Degi
<tpw_rules>
re rfc 62, what happens if you try to pass the same MemoryData to the constructor of two different Memory objects?
<tpw_rules>
i would suggest that one form of the Memory constructor be made a class method instead of having multiple argument sets and disambiguation logic
<tpw_rules>
(idk if there's precedent for that in amaranth but i've used it in my own python code)
<tpw_rules>
for example def Memory.new(cls, *args, **kwargs): return Memory(MemoryData(*args, **kwargs)) .
<tpw_rules>
Wanda[cis]: okay, i didn't figure there was anything useful for it to do, just wanted to make sure the problem was documented and understandable by the user
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] aaba255 - Deploying to main from @ amaranth-lang/amaranth@efcd9a45387eab744898690c9f601c7c17166441 🚀
<_whitenotifier-5>
[amaranth] github-merge-queue[bot] created branch gh-readonly-queue/main/pr-1238-efcd9a45387eab744898690c9f601c7c17166441 - https://github.com/amaranth-lang/amaranth
<whitequark[cis]>
<Chips4MakersakaS> "I saw some discussion about..." <- > <@fatsiefs:matrix.org> I saw some discussion about RAM ports, ASIC RAMs from a few days ago. I have worked on radiation-hardened ASIC SRAM compilers in my previous professional life. So some 2 cents from me.... (full message at <https://catircservices.org/_matrix/media/v3/download/catircservices.org/GbQtaePNoDIKwgFAjTlDtyXh>)
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] f6b7cce - Deploying to main from @ amaranth-lang/amaranth@cd51e02de27265b05e2596947ffdc2cc1158dbf6 🚀
<whitequark[cis]>
but yeah Memory and MemoryData are a little confusing
<anuejn>
what I wanted to say express is more:
<anuejn>
I think it is hard to teach newcomers how to use memories
<whitequark[cis]>
MemoryData is basically a Signal, and Memory is basically a way to get ports into MemoryData for RTL
<anuejn>
and why you have this exact trinity
<whitequark[cis]>
have you looked at the docs I wrote? they're universally regarded as very clear and understandable
<anuejn>
no, are there already docs for the new RFC?
<anuejn>
or just the rfc text itself?
<whitequark[cis]>
the docs are there for the current lib.Memory
<whitequark[cis]>
the MemoryData addition is a rather minor one; the only change is going to be "ok, put this in your constructor
<whitequark[cis]>
* your constructor"
<anuejn>
ah yeah that is the thing my opinion is about
<whitequark[cis]>
I'm not at all worried about it because if the current doc is extremely clear (which is the only type of feedback I got so far, at least from people who had any knowledge of HDLs at all, including "just read the guide") then a minor addition like MemoryData isn't going to make it hard to understand
<whitequark[cis]>
because all it's doing is saying "ok, put this thing in your constructor and that into elaborate" instead of "ok, put this thing in your elaborate"
<anuejn>
yeah
<tpw_rules>
whitequark[cis]: what did you think about having the "shortcut" constructor parameter set being a class function?
<tpw_rules>
i think you'd only need to do the split if you wanted to access the memory in sim? which is certainly a good thing
<anuejn>
tpw_rules: thats what I thought
<whitequark[cis]>
you usually want to access the memory in sim, I think
<whitequark[cis]>
re shortcut parameter set, we have similar function overloads already; Signal and Member both function a bit like that
<anuejn>
It has its own drawbacks (more ways of doing things) at the upside of being easier for beginners / in the simple case
<whitequark[cis]>
also, once we actually ship type annotations, your IDE will be able to show those overloads as two separate things
<anuejn>
whitequark[cis]: I rarely acess the contents of a memory in sim actually
<anuejn>
I would love to have that shortcut considered, but can also see why one would not want it
<whitequark[cis]>
ultimately this is just a matter of taste, I think
<whitequark[cis]>
there's no technical difference that I can see
<whitequark[cis]>
or semantic in general
<anuejn>
true
<anuejn>
but having the shorthand makes the change kinda backwards-compatible
<anuejn>
(if you dont use the memory in sim)
<whitequark[cis]>
oh, sorry, you're arguing for overloads, not against? I misunderstood you before then
<anuejn>
ah yes
<anuejn>
I do
<anuejn>
I am arguing for adding a set of constructor arguments that is compatible with the current set and creates the MemoryData internally
<whitequark[cis]>
that's what the RFC does i think?
<whitequark[cis]>
currently
<anuejn>
aah
<anuejn>
I did not understand that
<whitequark[cis]>
it's exactly for the reason you say, too (backwards compat)
<whitequark[cis]>
so you can pretty much just swap hdl.Memory for lib.Memory
<anuejn>
It does, but the guide level explanation section is somewhat misleading
<anuejn>
nice
<anuejn>
then I find the change completely reasonable
<anuejn>
I think that section shold also contain a notice about the overload
<whitequark[cis]>
tpw_rules: vscode, but anything that can use an LSP should be able to use pyright
<whitequark[cis]>
> <@libera_tpw_rules:catircservices.org> Catherine: what ide do you use? vscode?
<whitequark[cis]>
* vscode, but anything that can use a language server should be able to use pyright
<whitequark[cis]>
Wanda[cis]: after we have a complete replacement for it, I think
<Wanda[cis]>
anything more than RFC 58 + RFC <TBD for Format.Struct / Format.Enum>?
<whitequark[cis]>
I'd add that to "Future possibilities" and then we can deprecate it later as a minor change when our impl can handle everything the old impl did
<whitequark[cis]>
at least in principle
<tpw_rules>
so is the intention with memorydata that new designs will use it and the old parameter set is just for compatibility?
<whitequark[cis]>
you could use either; if you're really sure no one will ever need to access the memory in simulation you can use the old parameter set
<tpw_rules>
okay. i just noticed the old way was not mentioned in the guide
<whitequark[cis]>
um, what?
<whitequark[cis]>
hold on, what do you even mean by "old way" and "new way"?
<tpw_rules>
the guide section of the RFC says "the code to create a memory changes from `m.submodules += Memory(shape=..., depth=..., init=...)` to `self.data = MemoryData(shape=..., depth=..., init=...) ; m.submodules += Memory(self.mem_data)`
<whitequark[cis]>
oh, of the RFC
<tpw_rules>
yes
<whitequark[cis]>
yeah that's an ambiguity in the RFC
<whitequark[cis]>
I would document both ways as acceptable (since we don't deprecate either)
<tpw_rules>
fortunately i've been learning to read so i noticed the constructor documentation in the reference portion said the former way is still valid, so i wasn't sure if it was deprecated
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] de17bf0 - Deploying to main from @ amaranth-lang/amaranth@3d5c36a606a0eeb8982ce5c9b707351650b94695 🚀
<cr1901>
Just for transparency, I am in fact physically present for the meeting, but am distracted by other things. So prob won't be very active. Thx for addressing my comments yesterday
<Wanda[cis]>
and I'm going to add decoder deprecation to the "future possibilities" of the Format.Struct/Format.Enum RFC when I write it
<zyp[m]>
in #52, «if all value arguments are ShapeCastable, and it is the same ShapeCastable for all of them (as determined by eq on the ShapeCastable), the resulting value is transformed through ShapeCastable.call of that shape-castable»; when does this happen?
<Wanda[cis]>
oh.
<Wanda[cis]>
oh.
<Wanda[cis]>
oops.
<Wanda[cis]>
I think we have a problem here
<whitequark[cis]>
when the built-up Choice is cast to a value
<Wanda[cis]>
hmmm
<Wanda[cis]>
so it'd have to be a ValueCastable
<Wanda[cis]>
I think I wanted to avoid it, though not sure why
<whitequark[cis]>
yeah me too (also don't quite recall exactly why)
<Wanda[cis]>
oh
<whitequark[cis]>
actually
<whitequark[cis]>
we can just keep a running tally
<zyp[m]>
what if I make a Choice over a set of fixed.Value, whose .eq() looks at the expression type? it'd still be a Choice when passed to another fixed.Value.eq()
<Wanda[cis]>
I think it's because then you essentially cannot implement operators / methods on it
<whitequark[cis]>
like, whenever we have a pair of inputs, we also keep a cast result
<zyp[m]>
I think the easiest solution would be to add another method to explicitly finish the case chain and give you back a valuelike
<Wanda[cis]>
aaaaactually it kind of works
<Wanda[cis]>
that method is .as_value()
<whitequark[cis]>
oh, huh.
<zyp[m]>
that sounds wrong
<Wanda[cis]>
... kind of yes
<whitequark[cis]>
huh?
<Wanda[cis]>
I hm
<whitequark[cis]>
why?
<Wanda[cis]>
don't like this
<zyp[m]>
at least in my head .as_value() is for going from a value-castable to a value, not to a value-castable
<Wanda[cis]>
Catherine: building intuition for it will be annoying
<whitequark[cis]>
zyp[m]: that is incorrect
<Wanda[cis]>
like, most of the time you can just skip the .as_value() because you pass this thing to an .eq() or something else that will do Value.cast
<Wanda[cis]>
but sometimes you cannot
<whitequark[cis]>
.as_value() can, and often will, return a value-castable
<zyp[m]>
right, so Choice is effectively another layer of value-castable on top of the inputs
<whitequark[cis]>
this is how you can compose value-castables, in fact, and delegate functionality to another one
<whitequark[cis]>
it's pretty key to how the whole thing works
<zyp[m]>
and .as_value() strips the top layer
<whitequark[cis]>
yeah
<whitequark[cis]>
same for .as_shape()
<zyp[m]>
right, so a value-castable that looks at the type could special case for Choice and call .as_value() until something else falls out
<whitequark[cis]>
you probably would not want to special-case Choice just for UX reasons
<anuejn>
whitequark[cis]: for 59 I am not sure if I like it
<anuejn>
I use upwards clock propagation intentionally for cases where I have like an ADC and a submodule that is clocked by a physical pin of it
<whitequark[cis]>
anuejn: without it I'm not sure if we can do any clock domain rework
<whitequark[cis]>
the answer is "probably not"
<whitequark[cis]>
I also don't think I can teach this
<zyp[m]>
anuejn: you can define a clock domain higher up in the hierarchy than where you're driving it
<anuejn>
okay yeah, I see why it is nescessary
<whitequark[cis]>
the actual semantics is completely batshit, which is the real problem
<anuejn>
and I am fine with merging the rfc
<whitequark[cis]>
clock domains are sometimes being renamed automatically with the submodule name incorporated in them
<anuejn>
and deprecating the behaviour
<whitequark[cis]>
and if the submodule isn't named it becomes a hard error
<anuejn>
but I would like to have some kind of alternative before removing upwards propagation entirely
<zyp[m]>
whitequark[cis]: that's caused me a bunch of hassle in migen
<anuejn>
for 58 I dont really have opinions
<zyp[m]>
especially the «sometimes» part
<whitequark[cis]>
anuejn: define a clock domain at the highest level you can (ie in your ADC peripheral) and drive it from within the PHY
<anuejn>
Ah yeah that is a posibility
<whitequark[cis]>
it actually makes way more sense
<whitequark[cis]>
because to a reader it is clear that a domain is being created and propagated down
<anuejn>
it is incompatible with what I am currently doing but probably much saner
<whitequark[cis]>
rather than having to examine every submodule of everything in the hierarchy ever to see what the domain at any arbitrary point of it is
<anuejn>
yeah I totaly see why upwards propagation stinks
<mcc111[m]>
I am confused by daylight savings time. Is the meeting in five minutes or one hour and five minutes?
<tpw_rules>
last week it was five minutes from now
<tpw_rules>
the meeting is 1700 UTC and it's currently 16:57 UTC
<mcc111[m]>
thanks
<tpw_rules>
apparently next week there will be some more dst changes so that may change for you
<tpw_rules>
(but UTC won't of course)
Guest99 has joined #amaranth-lang
<tpw_rules>
connect is shape agnostic right? so with streams we can have a fifo with a custom shape in some sense
<Wanda[cis]>
correct
<Wanda[cis]>
we don't actually need custom shape support in fifo to connect custom shapes there
<Wanda[cis]>
it's just... a tiny bit inconvenient
<whitequark[cis]>
good evening everyone, it is the time for our regularly scheduled core subsystem meeting. who is attending?
<tpw_rules>
(i still would like to see fifo support custom shapes but that's cool)
<Wanda[cis]>
meow.
<tpw_rules>
why is it inconvenient?
<tpw_rules>
hello, i'm here for about 45 mins
jfng[m] has joined #amaranth-lang
<jfng[m]>
o/
<Chips4MakersakaS>
o/
<galibert[m]>
hi, just arrived
<mcc111[m]>
I am here, I may have to leave partway through the meeting but if I do I will say so
<whitequark[cis]>
please respond with your comments or proposed disposition: merge or close
<zyp[m]>
merge
<galibert[m]>
like it, merge
<tpw_rules>
question about transfer rule 3: is the following snippet valid according to that: valid = Signal(init=0) ; m.d.comb += valid.eq(1) ?
<jfng[m]>
merge
<galibert[m]>
need to try to actually use it to form a more refined opinion
<whitequark[cis]>
tpw_rules: valid, but not recommended
<whitequark[cis]>
(it's possible that some of the edge cases in the interaction of these rules will be refined during/after implementation)
<tpw_rules>
whitequark[cis]: ok, for that literal case i would use Const(1) indeed
<tpw_rules>
i say merge
<tpw_rules>
(sharing galibert[m]'s note)
<Chips4MakersakaS>
merge
<mcc111[m]>
My only question is: imagine one wants to use stream with an existing axi or wishbone component. the RFC says strict axi compatibility is not contemplated for several reasons including licensing. If one wants it, what does one do? Just implement it oneself? Could there be a third party library for this (if licensing precludes putting an adapter in the stdlib)?
<mcc111[m]>
But I think I vote merge regardless of the answer to that question
<Wanda[cis]>
merge
<whitequark[cis]>
since you brought it up in this particular way: ask a lawyer
<Chips4MakersakaS>
Is not clear to me if there is any relationship with bi-directional address mapped buses or it should be seen as totally different.
<whitequark[cis]>
ok, disposition on RFC 61: merge 🎉
<mcc111[m]>
Okay fair enough
<galibert[m]>
I don't think there's anything at all bidirectional in there
<jfng[m]>
it could work in a point-to-point link, with some bus-specific fsm or something
<whitequark[cis]>
Chips4Makers (aka Staf Verhaegen): AXI4 is five streams in a bundle
<tpw_rules>
is axi one of those things where you need a license to stamp axi compatible on your thing? like SD and HDMI?
<whitequark[cis]>
(in different directions)
<zyp[m]>
mcc111[m]: the rules we don't comply with are stuff like the width restrictions, in which case you just keep to those additional restrictions when designing a stream to be compatible
<whitequark[cis]>
tpw_rules: read the license on AXI-Stream please
<whitequark[cis]>
please note that this RFC closes an expressivity hole that currently makes it impossible to use memories in simulation (even with helpers on Memory) if you strictly follow all of the rules we lay out in our docs (namely, that of not mutating self in elaborate directly or indirectly)
<whitequark[cis]>
so the options are "merge" or "amend (with what?) and merge"
<whitequark[cis]>
s/with what/how/
<galibert[m]>
I've just got a question, why? Why this particular organization specifically, MemoryData in the constructor and Memory component in the elaborate?
<Chips4MakersakaS>
Can't __init__ signature be kept the same and have a memory.data field added inside this function ?
<whitequark[cis]>
I think the message I wrote while the two of you were typing answers that, actually
<whitequark[cis]>
Memory is mutable in a way that necessitates it be confined to elaborate, or else creating read and write ports in the constructor and referring to those
<tpw_rules>
i wish it could be less clunky. can we just move Memory to the constructor and have a .get_elaboratable() method which returns some internal object to avoid introducing another user type?
<whitequark[cis]>
if you don't know until elaborate how big your memory should be, which is fairly common, then you just can't do the latter
<whitequark[cis]>
tpw_rules: this doesn't seem to be less clunky to me
<whitequark[cis]>
it's just clunky in a more unusual way
<Wanda[cis]>
whitequark[cis]: in that case `MemoryData` won't save you either
<tpw_rules>
how does this solve that? wouldn't you have to modify the MemoryData?
<zyp[m]>
whitequark[cis]: but then you can't create `MemoryData` in the constructor either
<whitequark[cis]>
actually, yeah, good point
<tpw_rules>
it reduces clunkiness by the metrics of less user visible types and less objects to interact with. it's still weird indeed
<whitequark[cis]>
no, you have the same amount of user visible types and same amount of objects to interact with
<whitequark[cis]>
you're just making the type of lib.memory.Memory private, therefore making it hard to write docs for it
<whitequark[cis]>
that's the only real difference
<galibert[m]>
so you need to create all submodules in the constructor (if you want to see them in sim) except the memories?
<Wanda[cis]>
Chips4MakersakaS: I'd like to point out that this is actually supported by the RFC
<Wanda[cis]>
creating the MemoryData separately is an option, not a requirement
<Wanda[cis]>
if you use the existing constructor, it'll materialize a MemoryData for you and you can access it via the .data property
<tpw_rules>
whitequark[cis]: do you call that type lib.memory.Memory user visible and interacted with?
<Chips4MakersakaS>
Good; was not clear to me from the text as it says you have to change the code to allow to use it during simulation.
<Chips4MakersakaS>
> If the memory is to be accessible in simulation, the code to create a memory changes from
<zyp[m]>
personally I don't see a whole lot of value in creating the MemoryData earlier than the Memory, but I see a lot of value in being able to get it from .data and use it in simulation
<galibert[m]>
Chips4Makers (aka Staf Verhaegen): because you're not supposed to put the Memory object in self, so you have no way to reach it from outside
<tpw_rules>
can you simulate un-elaborated things? is it okay to nab the MemoryData from .data and put it in self during elaborate?
<tpw_rules>
i think no because you would get two different MemoryDatas
<galibert[m]>
Since you're supposed to create memories in .elaborate and not change self there iirc to allow doing elaborate multiple times on the same object
<whitequark[cis]>
tpw_rules: there is *some* type of the result of `.get_elaboratable()` which you call methods on. therefore it is visible (it appears in your code) and you interact with it (call methods)
<whitequark[cis]>
tpw_rules: it is never OK to mutate `self` during elaboration
<Wanda[cis]>
tpw_rules: generally `elaborate` shouldn't mutate the elaboratables
<whitequark[cis]>
except for debugging where you'd otherwise just use a global variable or something similarly dirty
<whitequark[cis]>
and even then you might regret it depending on how it goes (like how cr1901 found out)
<Chips4MakersakaS>
galibert[m]: Got it!
<tpw_rules>
ah, you couldn't move getting ports to the constructor-created object as that would break multiple elaboration again
<tpw_rules>
so i drop my idea
<tpw_rules>
zyp[m]: then i'm not sure your path works
<whitequark[cis]>
for traces=[], I think we should eventually do something like a selector expression, like "give me all traces for *.*_fifo.*" and then the simulator figures it out
<whitequark[cis]>
but for interacting with the memory, you still need a series of python attribute accesses
<whitequark[cis]>
* but for reading/writing the memory from a testbench, you still need a series of python attribute accesses
<galibert[m]>
How useful multiple elaboration actually is?
<whitequark[cis]>
lets you call Simulator(dut) multiple times, even in parallel in several threads, in testbenches
<whitequark[cis]>
like if you use pytest fixtures
<galibert[m]>
Ah
<Chips4MakersakaS>
Chips4MakersakaS: No wait, does it mean that mean that you may want to run the simulation without having the actual memory generated in `elaborate`?
<galibert[m]>
in the constructor, not in elaborate
<whitequark[cis]>
you would create MemoryData separately in the constructor
<whitequark[cis]>
which is why it's separate
<Chips4MakersakaS>
OK for me.
<jfng[m]>
i'd do `self._memory = Memory(...)` in the constructor, alongside other submodule instances, then assign them to `m.submodules` in elaborate
<jfng[m]>
and iirc, read_port is not a submodule, so that works
<whitequark[cis]>
it doesn't
<whitequark[cis]>
because .read_port() mutates self._memory which means it by extension mutates self
<galibert[m]>
I think it's going to be problematic longer term to have one specific Elaboratable type that's magic and the others requires to go out of their way to be multiply-elaboratable
<whitequark[cis]>
* because self._memory.read_port() mutates self._memory which means it by extension mutates self
<whitequark[cis]>
galibert[m]: this is actually not unique to `Memory` and there are similar challenges with other mutable Elaboratables, like a bunch of the interconnect in amaranth-soc
<whitequark[cis]>
which solves it in the same way (.freeze() then raise if frozen)
<tpw_rules>
what happens if you use the same MemoryData for two Memories? Wanda[cis] said earlier that it creates (in spirit) a DriverConflict but wouldn't that be the case if you try to elaborate twice?
<whitequark[cis]>
tpw_rules: think of `MemoryData` as a 2d `Signal`
<Wanda[cis]>
tpw_rules: not when the two `Memory` are in distinct elaborations
<tpw_rules>
or would the fact that the module tree is unique make that okay?
<zyp[m]>
galibert[m]: the reason this is not an issue for most regular elaboratables is because they're not mutated after construction
<whitequark[cis]>
you can use the same Signal in two different designs, it works just fine
<tpw_rules>
okay, thanks, that makes sense
<Wanda[cis]>
DriverConflict is when the same signal is driven twice within a hierarchy; it's fine if the whole hierarchy is elaborated twice, because the two drivers never meet
<mcc111[m]>
would it make sense for there to be some type-level-ish way to distinguish "always frozen" vs "must be frozen" elboratables? like a Freezable subclass of elaboratables, or a field on elaboratable?
<mcc111[m]>
not sure if i asked that in a way that makes sense
<zyp[m]>
galibert[m]: yes, and conversely the `DeclarativeMemory` suggested in #45 wouldn't have this problem
<Wanda[cis]>
how... would you even implement __setitem__?
<whitequark[cis]>
tpw_rules: you use it with `sim.set(mem.data[0], blah)`
<whitequark[cis]>
__setitem__ is impossible to implement since there's no way to pass in sim
<whitequark[cis]>
s/mem.data/mem_data/
<tpw_rules>
where 0 could instead be an arbitrary Value?
<Wanda[cis]>
the only valid use for indexing MemoryData is sim.get(data[idx]) or sim.set(data[idx], ...)
<tpw_rules>
then isn't supporting that kinda necessary?
<Wanda[cis]>
with a bunch of variants, like slicing the cell for masked writes
<whitequark[cis]>
tpw_rules: no I don't think so, `sim.set(mem_data[sim.get(idx)], 0)`
<tpw_rules>
yeah i was about to type that
<tpw_rules>
so there would have to be some sort of complex deference because then __getitem__ couldn't have sim either to just do that
<whitequark[cis]>
it doesn't need to; it returns a MemoryData.Row object that captures the memory and the index
<whitequark[cis]>
* it doesn't need to; it returns a MemoryData.Row object that captures the memory data and the index
<tpw_rules>
could you do x = mem_data[idx] ; sim.set(idx, 0) ; sim.get(x) ; sim.set(idx, 1) ; sim.get(x) ?
<whitequark[cis]>
no, it doesn't capture a value (which would be pretty confusing in the example you give)
<whitequark[cis]>
s/value/`Value`/
<whitequark[cis]>
just an int
<Wanda[cis]>
there's an edge case btw, __getitem__ with Value is strictly more powerful: you could do await sim.changed(mem_data[idx])
<jfng[m]>
what happens in `data` and `shape=`, etc are both passed to the `Memory` constructor ?
<jfng[m]>
hard error ?
<whitequark[cis]>
that's mentioned in the RFC text
<whitequark[cis]>
Wanda[cis]: yeah, and I don't think we can justify that power
<whitequark[cis]>
it sounds unimplementable in CXXRTL
<Wanda[cis]>
mhm
<jfng[m]>
ah right
<whitequark[cis]>
tpw_rules: so that's your answer: I don't think `__getitem__(idx: Value)` should ever be a valid thing because it would be problematic for cxxsim
<tpw_rules>
whitequark[cis]: i don't understand, wouldn't the returned Row in x have both the memory and the Value ? i mean "could you do" in the hypothetical world where __getitem__(Value) exists
<tpw_rules>
if you don't like that possibility (frankly it is confusing to me too), then that's another reason to put that to bed
<whitequark[cis]>
tpw_rules: yeah I don't think we should do this ever or at least before cxxsim is merged
<whitequark[cis]>
it sounds rather unimplementable
<Wanda[cis]>
yeah sounds good
<whitequark[cis]>
anything else?
<whitequark[cis]>
we have 20 minutes left
<tpw_rules>
okay. it all looks good to me. i say merge
<galibert[m]>
I don't like it but merge
<jfng[m]>
abstain
<whitequark[cis]>
merge
<Wanda[cis]>
merge
<zyp[m]>
I already said merge, but I'll repeat myself
<jfng[m]>
merge, makes designs easier to understand
<Wanda[cis]>
merge
<tpw_rules>
i have to head out now: 59 merge, it's problematic and workaroundable post-merge. 58: abstain, 52 mild merge, seems okay but haven't thought super hard
<Chips4MakersakaS>
merge
<whitequark[cis]>
galibert?
<galibert[m]>
Leaning merge, but thinking about how to bring things up upwards. What happens if you do m.domains += self.clockgen.cd_whatever ?
<whitequark[cis]>
that works fine
<whitequark[cis]>
or you can give clockgen the name of the domain and it does ClockSignal(name).eq(...)
<galibert[m]>
so you don't even need to pass a clock domain in the clockgen constructor?
<galibert[m]>
you can just pick it up either directly or better through a method?
<whitequark[cis]>
a clock domain object? no, you don't need to
<whitequark[cis]>
yes
<galibert[m]>
YOu could have that as an even easier alternative in Drawbacks. Merge
<zyp[m]>
how cursed is it to have a method on the clockgen that gets a reference to toplevel's Module and injects domains into it?
<whitequark[cis]>
awful
<whitequark[cis]>
ok, disposition on RFC 59: merge
<Wanda[cis]>
feels... moderately wrong
<galibert[m]>
Because in fact you already have ImportDomain, it's just m.domains[whatever] = ...
<Wanda[cis]>
that'll trigger if you do f"{valuecastable}"
<whitequark[cis]>
merge
<Wanda[cis]>
(also I'd like to point out that there's bikeshedding to be done on the conversion specifier tentatively called !v)
<zyp[m]>
!v sounds good to me
<whitequark[cis]>
oh yeah, any comments on the conversion specifier? it has to be a single letter
<zyp[m]>
and it should call Value.cast()
<Wanda[cis]>
also as for whether it should be Value.cast or .as_value(): unfortunately you cannot stack conversions, so we cannot make !v be .as_value() and have !v!v work as .as_value().as_value()
<jfng[m]>
!cv for casting, !lv for lowering, !av for as_value ?
<whitequark[cis]>
it has to be a single letter.
<Wanda[cis]>
jfng: it must be a single letter
<jfng[m]>
ah
<Wanda[cis]>
due to how the parser works
<galibert[m]>
!vvvvvv (as many vs as you need levels ;-) )
<zyp[m]>
I think if you want .as_value() you do it explicitly when you pass in the argument
<Wanda[cis]>
(also I'm not sure what you mean by lowering)
<jfng[m]>
(!l is "lower to a value")
<galibert[m]>
You have !av in the alternatives, that's a mistake?
<Wanda[cis]>
oh
<galibert[m]>
Maybe !v for single level, !V for full recurse?
<Wanda[cis]>
I thought you were proposing three specifiers for three different things
<Wanda[cis]>
nevermind
<whitequark[cis]>
galibert: sounds incredibly confusing if you have any amount of sight issues
<whitequark[cis]>
at least a and A differ in their shape, v and V do not
<Wanda[cis]>
my opinion: should be Value.cast, mildly leaning towards !v but don't particularly like it
<galibert[m]>
nN?
<mcc111[m]>
i'm struggling to understand when .as_value() would be specifically more desirable than Value.cast
<whitequark[cis]>
maybe just not include it at all?
<mcc111[m]>
if it's not included, then users would pick the "raw" specifier and call as_value() by hand?
<galibert[m]>
Yeah, kinda agree with mcc in fact, don't see how single as_value() can work
<zyp[m]>
another character for .as_value() can always be added later if somebody finds it desireable
<mcc111[m]>
agree with zyp
<Wanda[cis]>
conversion specifier is syntax sugar: you can always just call .as_value() or Value.cast() on the argument before passing it to formatting
<whitequark[cis]>
is anyone opposed to just leaving !v (regardless of character) out?
<Wanda[cis]>
it adds no expressive power
<whitequark[cis]>
then we do not have this problem at all
<zyp[m]>
I think I'd like to have it
<mcc111[m]>
i think having sugar for Value.cast() is desirable
<Wanda[cis]>
I wanted it so you can easily write Format("{x} (raw: {x!v})", x=my_enum_signal)
<zyp[m]>
it's useful in the same way as the normal !r
<zyp[m]>
and I don't see adding it is something we could regret
<whitequark[cis]>
!b for "cast to bits"? then we don't have to guess how many .as_value() there are; it's all of them
<zyp[m]>
that works, #51 already set a precedence for using «bits» about the raw value of a value-castable
<whitequark[cis]>
anyone opposed to !b?
<mcc111[m]>
to be clear you're suggesting !b but it's the same as the rfc's !v?
<whitequark[cis]>
yes, we are only talking about the character after !
<zyp[m]>
the only disadvantage I see is potential confusion between !b and :b
<mcc111[m]>
zyp[m]: that… actually is a *little* unpleasant, wouldn't you under some circumstances want to say both !b and :b then? for cast to bits, display in binary?
<whitequark[cis]>
r!b:#010b usually
<Wanda[cis]>
!b:b would indeed be a valid and useful thing
<Wanda[cis]>
hmm
<mcc111[m]>
i think my vote is for !v but if !b would be more consistent with something you did elsewhere then i don't feel strongly about !v
<whitequark[cis]>
so, we're in an interesting situation where we ran out of meeting time, everyone present said "merge", but we also have unresolved questions we're discussing. this hasn't happened before
<Wanda[cis]>
should we ... have auto-sizing of binary output? we do have the required information
<galibert[m]>
I can't make my mind whether that's programming in APL or in emojis
<Wanda[cis]>
(it's okay I'll shut up until after the meeting)
<whitequark[cis]>
Wanda[cis]: maybe that should be the default `Value` formatting
<mcc111[m]>
i had an opinion about #52
<whitequark[cis]>
we aren't discussing #52
<zyp[m]>
whitequark[cis]: we're bikeshedding a single letter, I don't much care what we end up with :)
<jfng[m]>
i have to go
<whitequark[cis]>
ok, disposition on RFC #58: merge
<Wanda[cis]>
also, just to add one more vote: merge, Value.cast, and I don't particularly care about the conversion char
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] whitequark 078e058 - Deploying to main from @ amaranth-lang/rfcs@f1829961a492265f8f606aa72dc1084195f29d7e 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] whitequark 7f859ea - Deploying to main from @ amaranth-lang/rfcs@055ffd6e795d328ca086a51f977cf9077f543ca3 🚀
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] whitequark 334db60 - Deploying to main from @ amaranth-lang/rfcs@98600aa4d75311e19bacc51627cbeb55dfa50fd5 🚀
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] whitequark 1cee981 - Deploying to main from @ amaranth-lang/rfcs@f312f6802958e9ac590412a9b8aa6872b7b0c4f4 🚀
<Wanda[cis]>
mcc111: I'm interested in your opinion
<_whitenotifier-5>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] d082861 - Deploying to main from @ amaranth-lang/amaranth@6ffafef794bf3cf8fe5ee93bcd111c86579db68d 🚀
<_whitenotifier-6>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] 85a6004 - Deploying to main from @ amaranth-lang/amaranth@fa2adbef84ca173d3c19fba37cfd7a3a042635f5 🚀
<zyp[m]>
I've got a question not directly related to amaranth, but since it was here I got PDM recommended, I'll ask it here anyway; how do I use it effectively to work on multiple packets in parallel? the documentation states «Editable packages are allowed only in development dependencies» and I've got a couple of runtime dependencies I'd like to edit