<GitHub-m-labs>
[artiq] jordens commented on issue #1207: In words: Take two "devices" C (or Urukul) that implement a bit of functionality in different ways using either device A or B (`TTLOut` or something else). How can that be written? https://github.com/m-labs/artiq/issues/1207#issuecomment-446508663
cr1901_modern has quit [Read error: Connection reset by peer]
futarisIRCcloud has quit [Quit: Connection closed for inactivity]
<GitHub-m-labs>
[artiq] jordens commented on issue #1207: @whitequark The MCVE is the first snippet posted. It's complete, verifiable and the most minimal I could come up with. Forget about the second snippet.... https://github.com/m-labs/artiq/issues/1207#issuecomment-446375417
_whitenotifier has joined #m-labs
<_whitenotifier>
[GitHub] Approachable is better than simple.
<_whitenotifier>
[m-labs/nmigen] whitequark pushed 1 commit to master [+0/-0/±4] https://git.io/fp5zV
<_whitenotifier>
[m-labs/nmigen] whitequark 851ed06 - ClockDomain.{rst→reset}, for consistency with ResetInserter.
<rjo>
i wonder if it's not better to make make the clock domain the thing that statements are added to.
<rjo>
maybe also call it "control set" and include CE/RST to be closer to what FPGAs call that thing.
<whitequark>
so, migen's ClockDomain already includes rst
<rjo>
yes.
<whitequark>
I'm not sure if grouping CE into control set is really better
<whitequark>
IME Yosys will often pull CE from thin air and convert DFF to DFFE when it feels like it
<whitequark>
so there's no direct correspondence
<whitequark>
with reset, nmigen's ClockDomain has asynchronous reset, so that's a necessity
<whitequark>
I've added first class async reset to fix the bug you found with AsyncFIFO
<whitequark>
which I've also hit, and wasted a lot of time on.
<rjo>
i wonder if "self.sys += a.eq(b); self.comb += c.eq(d)" and having comb and sys be clock domains (comb being the usual combinatorial CD which is equivalent to a "very fast clock" from migen's perspective).
<whitequark>
I already have comb as a clock domain.
<rjo>
i really do like the context-style coding.
<rjo>
good.
<whitequark>
thanks. I spent quite a while devising the best possible DSL.
<whitequark>
there is now some checking of control set mismatch
<rjo>
i couldn't find the code where the context stack is attached to the statements and where it is traversed when serializing/emitting. where is that?
<whitequark>
e.g. this is a hard error: self.comb += a.eq(1); self.sync += Cat(a, b).eq(123)
<whitequark>
or self.sync.a += , self.sync.b +=
<whitequark>
the reason I do not completely segregate statements by control set is so that stuff you add in a single If stays together in the RTLIL/Verilog output
<rjo>
but just "driving a signal from multiple clock domains a precise, hard error." is not good. a properly coded and constrained CDC can look exactly like that and is not an error.
<whitequark>
what do you mean?
<whitequark>
driving different bits of one signal?
<rjo>
statements with inputs from multiple clock domains is not always an error.
<whitequark>
no, that's not what I meant by that phrase
<whitequark>
it is primarily catching an error where you would e.g. drive the same signal (all bits of it) from both comb and sync
<whitequark>
this is legal synthesizable Verilog for some absurd reason, and the synthesizer should just choose one of them... at least that's what Clifford told me
<rjo>
the rtlil output is fine. i only worry about locking migen into a yosys corner. can that rtlil2verilog converter be extracted and used by those who only want to get from migen to bitstream (e.g. windows + ise)?
<whitequark>
it is probably easier to just write a nmigen-ir-to-verilog converter and verify it against rtlil-to-verilog converter (e.g. signedness bugs and such) by using yosys' formal verification capabilities
<whitequark>
i.e. run equiv_make;equiv_induct on a set of designs
<rjo>
yes. having contradictory CDs on the assignment is wrong.
<whitequark>
that's what I mean, yes. driving a signal with a value *computed from* signals in other clock domains isn't currently detected nor an error
<whitequark>
though it could be detected, of course.
<whitequark>
I added some stuff to make writing analyses and transforms a bit easier.
<rjo>
ok. that separate converter sounds good.
<rjo>
having a way to locate/mark CDCs (kicking of migen#103) would be awesome.
<rjo>
and if that do_finalize() thing and the pitfalls (idempotency, simulation breaking) associated with it could go away, that's also nice.
<whitequark>
yes. and there are some bugs with finalize too.
<whitequark>
in nmigen it's just a tree of calls to get_fragment that sometimes request platform to give it a primitive.
<whitequark>
no magic.
<rjo>
"conversion of RTL to Verilog" is confusing. migen's FHDL isn't RTL to me.
<rjo>
and hierarchical designs are also extremely useful for readability of rtlir and verilog.
<whitequark>
sure.
<whitequark>
rtlil is not very readable because each aritmetics op is a 10 line cell insantiation.
<whitequark>
but write_verilog is ok.
<whitequark>
well, to get the output in README, it needs a yosys patch, it's not yet upstream.
<whitequark>
right now it's a bit uglier. but that's fixable.
<whitequark>
still, it avoids some rather nasty bugs like illegal verilog generated for certain slices on LHS
<whitequark>
signedness bugs too. hit another one just today.
<rjo>
how is the "implicit signedness" drawback handled? just defer to rtlil?
<whitequark>
in rtlil signedness is explicit
<whitequark>
so if there are any bugs in write_verilog or our verilog backend, equiv_induct should find them.
<rjo>
but in nmigen/migen signedness is implicit. the magic implicit-to-explicit transformation is still there, right?
<whitequark>
the problem is that the migen verilog backend assumes certain correspondence between verilog $signed and migen sign extension/promotion
<whitequark>
with nmigen rtlil backend, rtlil is forced to express the exact nmigen semantics (whatever it is)
<whitequark>
since it defers to nmigen's bits_sign for complex operations when determining which parameters to set in rtlil
<rjo>
i have to think about that.
<_whitenotifier>
[m-labs/nmigen] whitequark pushed 2 commits to master [+3/-0/±2] https://git.io/fp5wJ
<_whitenotifier>
[m-labs/nmigen] whitequark bc60631 - genlib.cdc.MultiReg: pull in from Migen.
<rjo>
there seems to be a lot of boilerplate still: when doing hierarchical stuff, the ports need to be created, named, and listed again in the conversion output. is there no way to add sensible defaults or compactify that syntax?
<whitequark>
definitely not in rtlil (it is designed to be explicit in that aspect), and probably not in verilog, at least not very nicely
<whitequark>
what more compact form are you thinking of?
<whitequark>
yosys has an extension that allows you to do `module foo(...);` where `...` causes it to infer the port list from input and output wires in the body, but it's not much and it's nonstandard
<whitequark>
that's all I can think of
<rjo>
also the get_fragment(platform), f=Module(); return f.lower(platform) pattern also looks repetitive to me. it'll look very similar in all classes.
<whitequark>
I agree on that one
<whitequark>
it's somewhat preliminary. I don't like it but so far I wasn't able to reduce it nicely to something else.
<whitequark>
any suggestions?
<whitequark>
this is essentially dependency injection, the way I removed special overrides...
<rjo>
re Signals and ports, if all the logic is generated in get_fragment() there seems to be no need to have self own the "internal" signal created in get_fragment(). and all the Signals attached to self in __init__ are ports, then why not use that info and enforce it?
<whitequark>
signals being a field on self is not magic in any way
<whitequark>
also, signals being (or not being) fields on anything is not considered when detecting ports
<rjo>
yeah. maybe field is not the correct trigger to mark a port. but there should be some other way to depend on/expose/leverage more rtlil/verilog module/port structure.
<whitequark>
I'm really confused as to what you're suggesting
<whitequark>
right now, what triggers conversion of a signal into a port is one of the two:
<whitequark>
a) signal is used across hierarchy
<rjo>
what are the signals that you are creating in __init__?
<whitequark>
b) signal is defined as a port explicitly, by passing an argument to convert
<whitequark>
merely the signals that i consider useful to have as fields on self
<whitequark>
there's no special semantics.
<whitequark>
e.g. if you look at current MultiReg, the internal DFFs are put in an array on self
<whitequark>
it should probably be self._regs. but that's minor.
<rjo>
yes. forget about the field-or-not distinction. i see that that's not helpful.
<whitequark>
if you put a signal into self.x but never use it outside of the current module, it will not become a port.
<whitequark>
ok.
<rjo>
i think i'd want to create the ports list in __init__ in some way.
<whitequark>
what is the goal?
<rjo>
self contained information. if the module changes and adds/removes ports, then I have to change code in one place only.
<whitequark>
but that's worse than what happens now?
<rjo>
i don't want to go to the verilog.convert() call and mess with that.
<whitequark>
nonono
<whitequark>
you misunderstood
<whitequark>
try this
<whitequark>
take alu.py, remove all ports from verilog.convert call
<whitequark>
it will generate:
<whitequark>
module top(a, b, sel);
<whitequark>
really, you only *have* to provide top-level outputs to verilog.convert.
<whitequark>
every port *inside* the hierarchy is automatically determined.
<whitequark>
of course, I would never make a design that requires you to manually list all ports inside hierarchy explicitly...
<whitequark>
try this on alu_hier too.
<whitequark>
actually, I think that one currently has a bug.
<whitequark>
but anyway, all ports inside the hierarchy are always automatically detected, you only feed verilog.convert the top level ports.
<whitequark>
so it can do some sanity checking of them.
<rjo>
ok. alu_hier actually shows that already.
<whitequark>
yes, but the wire widths are wrong, and it pulls too many ports out to the toplevel.
<rjo>
and then i guess at the top level, the information generated when constraining signals to actual platform pins is used to generate the top level ports automatically as well. i.e. when targetting a platform there is no need to spec top level ports either.
<whitequark>
yes.
<whitequark>
similarly you will not explicitly list clock domains.
<rjo>
ack. i still have qs and comments on the clock domains implementation. right now you make them at the top and map them by name in convert(). does that scale?
<whitequark>
no, of course not.
<whitequark>
the clock domains will work the same way they work right now in migen.
<rjo>
the case where you create a local clock domain (like good old migen with the multiple pix CDs).
<whitequark>
yes. I just wasn't sure how exaclty they work. sb0 explained this to me some minutes ago.
<rjo>
so what about getting rid of that "sync" notion and just put the name of the CD there?
<rjo>
f.sys += assignment
<whitequark>
I feel like it's more confusing in case sys does get renamed to something else
<rjo>
compared to the current state it only amounts to removing the "sync."
<whitequark>
ah, there's another issue
<whitequark>
it means we aren't free to add new attributes on Module later
<whitequark>
in case they clash with someone's clock domain name
<whitequark>
but more importantly, most modules don't really care what clock domain they run in
<whitequark>
so, you use f.sync, and then rename it to something else in an outer module
<whitequark>
replacing generic "f.sync" to "f.sys" seems unnecessarily specific to me.
<whitequark>
all in all i am ambivalent about this. i think semantic fixes and corner case fixes are more important in most ways.
<rjo>
exactly. in the standard coding style, call the "default sync CD" just "sync" (instead of "sys"). all other CDs are and will be used by name.
<whitequark>
ok, I see.
<whitequark>
that makes sense.
<whitequark>
what about the naming clash issue?
<rjo>
the idiomatic solution would be to have a dedicated thing where "assignments in contexts are added to clockdomains".
<whitequark>
if not i'll just keep implementing nmigen.
<rjo>
having a compact and readable assignment syntax is very important to me. i'd see that as a far reaching issue.
<rjo>
other than that, i'd really like to get a fast simulator back.
<whitequark>
i am considering the assignment syntax issue you raised a blocker for any sort of public release. good enough?
<rjo>
i don't think the old iverilog thing was all bad.
<whitequark>
regarding a simulator, i want to start with a python simulator that translates to python lambdas.
<whitequark>
instead of the horrifyingly slow migen.sim
<rjo>
sure. that assignment syntax and the boilerplate thingy in and around get_fragment().
<whitequark>
ack.
<whitequark>
i want iverilog too. i briefly looked at VPI.
<whitequark>
i am more interested in it for cosimulation with verilog instances.
<whitequark>
like mor1kx.
<rjo>
i suspect if you want to stick with the simulator core in python, you want to add a dirty expression tracker that speeds up the delta cycles.
<whitequark>
i think a simulator in python is unavoidable
<whitequark>
i am sure that vpi+iverilog is a complete nightmare on windows.
<rjo>
yes. doesn't yosys do anything for simulation? to me such a deep insight into the RTL and simulation are very related.
<whitequark>
yosys has an evaluator
<whitequark>
it is very bare-bones and i'm not sure if it is very fast, it seems unlikely
<whitequark>
it doesn't track any state, anyway.
<rjo>
that sensitivity graph was migen#69
<whitequark>
in case of the simulator, i think what is more important is to have a clean design on top of which a good simulator can be built
<whitequark>
i don't want to block nmigen on a very fast simulator.
<whitequark>
the migen sim code is quite gnarly. i think i can avoid that.
<rjo>
sure. that's not a blocker at all. just when a simulator materializes, it should not be crappy.
<whitequark>
well, i think it is very easy to do better than migen.sim
<whitequark>
not calculate bits_sign on every cycle for every signal, for example.
<whitequark>
it should be possible to compile the entire frag.statements into a single python function
<whitequark>
assuming comb loops are detected, you only calculate it once
<rjo>
and a few syntax nits: maybe s/get_fragment/make/ or something similar. there is that set of terminology that should be cleaned up or explained "fragment" vs "module", "submodule"
<whitequark>
it used to be .build()
<whitequark>
I don't remember why I renamed it. I'm not married to .get_fragment().
<rjo>
right. if you talk about compiling into a single function, then at some point i'd want to give cython or some other JIT a try. but also later.
<whitequark>
llvmlite?
<rjo>
if it is get_fragment, i want to know what exactly a fragment is.
<rjo>
yeah. or any of the other paths.
<whitequark>
a module is the representation of logic in the front end. a fragment is the representation of logic in middle end.
<whitequark>
an rtlil process is the representation of logic in the rtlil back end.
<whitequark>
fragment ~ ir
<whitequark>
module tracks concerns like "are we in the middle of an If". fragment doesn't care.
<rjo>
oh. and do add a FSM example to show of how that would look (sync assignments in states, what would/could become of before_entering(), etc).
<whitequark>
module doesn't care about ports. fragment cares.
<whitequark>
yes. I need to add FSMs. it would look like
<whitequark>
"with f.fsm" would not be nestable in an If
<rjo>
how do i specify which CD the FSM updates next->state?
<whitequark>
mm, good point
<whitequark>
it should not be splattered over each instance of f.next.
<rjo>
f.sync += f.next("FIRST") maybe?
<rjo>
well. maybe. but since you are detecting conflicting CD in assignments, you'll find that error quickly.
<whitequark>
but then you get a huge noisy diff and you need to search and replace.
<whitequark>
seems like busywork.
<whitequark>
i would personally do "with f.FSM(state, cd="pix"):"
<whitequark>
I think.
<rjo>
also it could be two different CDs with the same RST and CLK, one with CE inserted the other without.
<whitequark>
or I guess the convention is "f.FSM(state, domain="pix"):'
<rjo>
yeah. probably fine.
<whitequark>
ok.
<whitequark>
as for "before_entering" etc.
<rjo>
add "next" and "state" to the list of potential clashes with clock domain fields.
<whitequark>
with f.before_entering("NEXT"):
<whitequark>
and so on.
<rjo>
ack the general notion of s/clock_domain/domain/
<whitequark>
I can concede the assignment syntax to you if we rename "clock domain" to "domain" in it. `f.domain.comb +=`, `f.domain.sync +=`, shortened to `f.d.comb +=`, `f.d.sync +=`
<whitequark>
since "comb" is definitely a domain of some sort.
cr1901_modern has joined #m-labs
<whitequark>
actually, I rather like it this way.
<whitequark>
there's some nice orthogonality.
<rjo>
i was excited way back when i added those four methods to FSM. nowadays I am not so exited anymore. but if those look like CE in your example, that's perfect.
<whitequark>
there is a fairly major issue with those methods.
<whitequark>
i've tried using them for a thing in my CPU.
<whitequark>
the problem is that migen generates blocking assignments in that huge always@(*) block, but the ordering between before_entering and state (e.g.) is not well defined
<whitequark>
so, if you e.g. set up some state in .before_entering, and access it in .state(), you're setting yourself up for a bug.
<rjo>
yes. you should never feed back from those signals onto state.
<whitequark>
maybe they are better deprecated then.
<rjo>
but i think the functionality is needed. just that the usage has that big pitfall.
<whitequark>
well.
<whitequark>
if FSM is a primitive and not a submodule with finalization, i think i can make it work.
<rjo>
if nmigen resolves the pitfall, that's fine.
<whitequark>
ok.
<rjo>
and another nit, make the coding style "m = Module()" and not "f". that's confusing.
<whitequark>
ah, yeah.
<whitequark>
leftovers from an earlier design.
<rjo>
is there aactually use case for making multiple (top) Modules in get_fragment() and merging them to return a single fragment?
<rjo>
i.e. not by using submodules.
<whitequark>
no. but there is a use case for not making a Module in get_fragment at all.
<rjo>
if a single module is always sufficient, i'd prefer the old style where the classes inherit from Module.
<whitequark>
so XilinxPlatform would define get_multi_reg().
<whitequark>
I would like to avoid inheriting from Module. take a look at Module.__getattr__.
<cr1901_modern>
I don't understand why one would want multiple top Modules in the first place
<rjo>
whitequark: ack.
<rjo>
but then what does Module actually do? you call it "representation of logic in the frontend". it owns only assignments (and their domains and context), right?
<whitequark>
it owns the state machine that handles If/Elif/Else/Case/FSM...
<whitequark>
it owns the DSL, essentially.
<whitequark>
if you want to factor out some of your If's into a function, you can do that but you pass `m` inside.
<whitequark>
and it's composable, too.
<whitequark>
making this magic-less relies on m being explicit.
<whitequark>
sb's design had implicit m and IMO it had way too much magic.
<rjo>
ack. so it owns the assignments and the context stack (in GL lingo).
<whitequark>
mm, something like that.
<rjo>
and manages the stack. and does lower().
<whitequark>
the AST transformations are all pure (I hit a *ton* of mutability bugs with them), so there's no need for a clear "owner" of statements.
<whitequark>
lower is basically just flush and repackaging its contents into a Fragment, discarding frontend state.
<whitequark>
really, probably most aspects of nmigen design are driven by "I hit a ton of obscure bugs with this in migen"...
<rjo>
but Module does own that, "self._statements.append(assign)" etc.
<whitequark>
sure. i think i use a slightly different definition of "own" here.
<whitequark>
it doesn't matter.
<whitequark>
it holds the statements.
<whitequark>
AttributeError: 'Module' object has no attribute 'sync'; have you meant 'd.sync'?
<whitequark>
added nice error messages for this common error.
<whitequark>
something *sorely* lacking in migen.
<rjo>
yes!
<whitequark>
actually, all errors in nmigen that i wrote so far should be specific and descriptive.
<whitequark>
it really helps to approach this with my language designer hat.
<rjo>
also, once you look at the simulator, maybe using a standard async event loop is the right thing to do here. i.e. just build it on asyncio, even the internal loop/delta cycles. and could be really nice when adapting to VPI or whatever external simulator would be involved.
<whitequark>
i actually hacked asyncio into the simulator event loop.
<whitequark>
i needed it in glasgow, since glasgow is fully async based
<rjo>
nit: i think the natives would say "did you mean 'd.dync'" but i am happy to learn something new.
<whitequark>
ah, you're probably right. when i'm tired my english comprehension drops dramatically.
<rjo>
doing asyncio would probably be really useful. spawning new generators and collecting them would be much easier, integration into plotting etc, etc.
<whitequark>
mm, I am not so sure.
<whitequark>
I've found asyncio somewhat opaque and sometimes annoying.
<whitequark>
for example.
<whitequark>
hm ok nevermind, maybe it will work.
<rjo>
python is also opaque and annoying. that doesn't stop us. ;)
<whitequark>
but the nmigen.compat.* stuff will end up very gnarly.
<whitequark>
ha, that is true.
<whitequark>
i do not like python in the slightest.
<whitequark>
i guess it is less bad than javascript.
<attie>
error messages!
* attie
does a happy dance
<rjo>
do you mean using the nmigen.compat stuff or writing that layer?
<whitequark>
writing.
<whitequark>
specifically so that using it is painless.
<whitequark>
for most of the core parts, it will be not so bad.
<whitequark>
but asyncio integration promises to be fun, in a bad way.
<whitequark>
i'm pretty sure i can do it, i just hate the idea of debugging it.
<whitequark>
migen's simulator has enough footguns on its own that need to be fixed
<whitequark>
e.g. .eq on a comb signal is silently ignored (wtf?!)
<whitequark>
yield vs yield from
<whitequark>
rjo: ideas for nmigen's sim.
<whitequark>
`await sim.set(s, 1)` sets signal (reg) forever
<rjo>
oh. another style/organization thing: it would be great if the star imports could go. Module().If() is a big step. only "import nmigen as nm; nm.Module(), nm.Fragment(), nm.Signal(), nm.Domain()" left.
<whitequark>
`await sim.strobe(s, 1)` does this for one cycle
<whitequark>
(in the generator's domain)
<whitequark>
star imports in user code or within nmigen?
<rjo>
ah. yes. i was thinking not even doing a migen compat layer for the simulation stuff. just skip that.
<whitequark>
it's important to see if the compat layer works good enough for a user's design to switch
<rjo>
start with showing that user code looks nice without star imports. then maybe later clean up the internals.
<whitequark>
i like * in internals. i tried writing all of them out explicitly at first, and it's just busywork.
<whitequark>
so i want to keep them there, within reason.
<whitequark>
ack re user code.
<whitequark>
but it should already be possible.
<rjo>
yes. modulo the footguns hidden in the old simulator... but that's only a pain to the implementer, not so much a user who has working sim code.
<whitequark>
true
<rjo>
ack all. that's all i have for now. nice work!
<whitequark>
ack.
<_whitenotifier>
[m-labs/nmigen] whitequark pushed 2 commits to master [+0/-0/±10] https://git.io/fp5MT
<_whitenotifier>
[m-labs/nmigen] whitequark pushed 1 commit to master [+0/-0/±2] https://git.io/fp5Dh
<_whitenotifier>
[m-labs/nmigen] whitequark ad9b45a - fhdl.ir: fix port threading code.
<rjo>
whitequark: yes. like the the final width of state. OTOH maybe that should stay hidden because state is opaque.
<rjo>
isn't this a bit similar to the If/Else case where the Else is not syntactically associated with the initial If anymore?
<whitequark>
yes. if you intersperse anything between If/Else, be it f.d.sync +=, or Case, or anything, the link is broken
<whitequark>
and when you do Else, it crashes (with a nice error).
<rjo>
is that required or just a design/syntax decision?
<whitequark>
it is not technically required, and is done to catch bugs.
<rjo>
i could imagine that the same desire to create new Signals etc between two FSM Cases also arises betwen bare Case cases.
<whitequark>
in my experience, FSMs can get extremely large. many pages of text large.
<whitequark>
usually if your If or Case gets that large, it probably needs to be restructured.
<whitequark>
but you can't easily split an FSM.
<whitequark>
the FSMs are different in another way though
<whitequark>
in principle, nothing stops you from extending the FSM with cases just wherever, because the cases (except the very first one) are not order dependent
<rjo>
you don't split them but people refactor them to be readable. i.e. create state dependent logic in a function and return the new state.
<whitequark>
the same is not true about If/Elif/Else or Case though, which are inherently order dependent
<whitequark>
(Case is order-dependent because you can use don't care bits)
<rjo>
Case (at least without x and z) is not order dependen either.
<rjo>
the branch conditions don't overlap (apart from the default).
<whitequark>
the nmigen cases are like casez, except the don't care bits are *actually* don't cares
<whitequark>
(the "a" RTLIL bit state)
<whitequark>
this is used internally to expand If/Elif/Else, and is also very handy for writing certain conditions more clearly, so i think it is valuable to have them.
<rjo>
does that matter? is z exposed in nmigen?
<whitequark>
not right now.
<whitequark>
and probably should never be exposed.
<whitequark>
it only really matters when including migen code and verilog code together.
<rjo>
yes. so the synthesizer can decide how to implement the case/fsm.
<rjo>
then nmigen Case is in fact order indenepdent (apart from default).
<whitequark>
how so?
<whitequark>
you still have don't cares
<rjo>
in nmigen?
<whitequark>
yes
<whitequark>
see examples/pmux.py
<rjo>
but why is that? just for interop with verilog?
<whitequark>
for writing decoders
<whitequark>
e.g. instruction decoders. things like that.
<rjo>
hmm. ok.
<whitequark>
i've encountered the need for don't cares in cases occasionally
<whitequark>
i worked around that and i was very unhappy with resulting if/elif chains
<whitequark>
and they were usally buggy too.
<whitequark>
not to mention hard to understand.
<rjo>
ok. i guess that makes sense. they only occur in Case and If/Elif, right?
<whitequark>
If/Elif is sugar for Case
<whitequark>
the only control construct nmigen has internally is Case.
<whitequark>
(because that's the only one RTLIL has)
<rjo>
and is that string syntax for them good? no need to do dont-care in non-binary i.e. hex representations?
<whitequark>
it currently verifies them strictly i.e. anything not in "01-" is rejected.
<rjo>
do those wildcards make sense for FSM as well?
<whitequark>
we can always add a "0x" in front. or i can require "0b" in front right now.
<whitequark>
don't cares definitely make no sense for FSM because FSM states are enums, not bit strings or numbers
<rjo>
i feel that the strict binary syntax is ok. everything else (signedness) is probably a recipe for desaster. but what about destructuring records with dont-cares on fields?
<rjo>
but i do like the dont-care now. smells a bit like match in rust.
<whitequark>
yes. it is exactly like match.
<whitequark>
what is destructuring records? do we have that now?
<whitequark>
in migen, that is?
<whitequark>
do you mean (a) having records as first-class citizens so you can do rec1.eq(rec2) and (b) Case matching on records?
<whitequark>
i am strongly in favor of (a), and i would like (b) but implementing it is a bit of a pain
<rjo>
no. but now you have opened the can of worms. of matching in Case. specifically if you have an instruction decoder where you view the instruction as a Record, and then match on fields with don't cares on others. that sounds like what you'd want.
<whitequark>
i think having that sounds like a sensible future improvement.
<whitequark>
perhaps a project for an ambitious intern. or maybe i'll do it some day.
<rjo>
the "no" refers to "in migen?"
<rjo>
ack.
<rjo>
and ack the first class citizen with rec.eq(req1). absolutely.
<whitequark>
I fixed alu_hier.py by the way, it generates much more sensible verilog now.
<rjo>
what are the casting rules? Signal(8).eq(Record(a=Signal(3), b=Signal(5)))
<whitequark>
I think you need explicit .raw_bits() here.
<rjo>
right. Records are not ordered.
<rjo>
unfortunately that is.
<whitequark>
I thought they are.
<whitequark>
which is why their constructor takes a []
<rjo>
oh right.
rohitksingh_work has quit [Quit: Leaving.]
<whitequark>
aren't all dicts ordered in Python now, anyway?
<whitequark>
since 3.6 or something?
<rjo>
but then is there a risk with implicit casts?
<cr1901_modern>
we still support 3.5
<whitequark>
I think you should only be able to assign Record to Record of same shape, and Signal to Signal.
<whitequark>
everything else needs explicit cast.
<whitequark>
this is strictly better than the current situation, and seems very safe.
<whitequark>
and if we really want, it can be relaxed later. though I would probably not want any relaxation.
<whitequark>
Records being structural not nominative seems good, this lets you avoid the case where one ethernet core can't talk to other rgmii interface without casts even though shape is the same.
<rjo>
ack. maybe there is also a chance to find a better name for raw_bits. to_signal?
<whitequark>
sure.
<whitequark>
or just .bits()
<rjo>
if that can never be understood to mean "the number of bits"...
<whitequark>
I would rather rename "bits_sign()"
<whitequark>
which is a really ugly name indeed, and not even in the right order
<whitequark>
wait, or was it?
<whitequark>
ok, it's in the right order. but still ugly.
<whitequark>
.dimension() ?
<rjo>
numpy has "dtype" for that data type description. extends to heterogenous types, records, ndim arrays
<whitequark>
.shape() maybe?
<rjo>
yes. bits_sign is ugly.
<rjo>
so bits_sign, layout, width all renamed to shape?
<whitequark>
yes.
<whitequark>
where do we have .width() ?
<whitequark>
there is Signal.__len__.
<rjo>
i was also thinking of memory data/address width etc.
<whitequark>
I think those are just sizes of fields of the primitive.
<rjo>
in numpy __len__ only gives you the outermost dimension. i suspect in nmigen it should remain the overall number of bits.
<whitequark>
records probably should not have __len__ at all
<rjo>
hmm. sounds right.
<whitequark>
btw I just discovered Cat(reversed(s)) is valid
<rjo>
yep. i shoehorned most of the python iterable stuff into Signal.
<whitequark>
I also fixed the __bool__ casts in nmigen
<whitequark>
it explicitly uses a container with a wrapper so you don't need the __eq__ hack.
<whitequark>
so, now `if a == b` is a bug even if a and b are signals.
<rjo>
it is a bit unfortunate that the input and output of reversed() are very different. might prefer a proxy.
<rjo>
and i suspect for proper operation there will be a couple more proxy signals.
<whitequark>
ok, then I don't understand the need for a proxy.
<whitequark>
RTLIL natively represents Cat()
<whitequark>
you can do assign { a, b } { b, a } in RTLIL
<rjo>
i would like an easier way to make proxies for readability and to be able to add names to those Cat(Slice) things.
<whitequark>
oh, I see.
<whitequark>
sure. that can be done.
<rjo>
i realize that the comb assignment in proxy() is a bit hard to do without a handle on the comb domain.
<rjo>
maybe that's another speciality of the comb domain. it's globally available, singleton.
<whitequark>
i really don't like that.
<rjo>
the speciality or the proxy()?
<whitequark>
that just cancels out all orthogonality we got from unifying domains
<whitequark>
that speciality
<whitequark>
proxy() is fine.
<rjo>
ok. then scratch it.
<rjo>
one test corner case bug that i stumbled on a couple times goes like this: a=Record(); sync += a.raw_bits()[1:].eq(a.raw_bits()); sync += a.raw_bits()[0].eq(sdi); comb += sdo.eq(a.raw_bits()[-1])
<rjo>
typical serial shift register thing on a record. there are multiple uses of raw_bits on both LHS and RHS in both comb and sync domains.
<whitequark>
i don't think this is an issue with migen because lhs and rhs signals are completely separate.
<rjo>
this regularly angered me in migen. it should be valid.
<whitequark>
when "s" is used on lhs, it is actually "s$next" internally.
<whitequark>
and then after everything is said and done, it is actually assigned.
<whitequark>
so this should work.
<rjo>
a little complication on that use case is where you want to peek at the combinatorial next state of a.
<whitequark>
with Nmigen, sorry.
<whitequark>
right.
<rjo>
ok. great.
<rjo>
i think that little SR on a Record would also make a nice show case for nmigen.
<whitequark>
SR?
<rjo>
shift register
<whitequark>
ah. yeah.
<whitequark>
sure.
<whitequark>
rjo: what is the good way to do deprecations in python?
<rjo>
whitequark: re m.Case(s, "01-"): In If/Elif the usual m.Elif(s & 0b110 == 0b010) is not significantly more code. and doesn't loose much readability.
<rjo>
what am I missing?
<whitequark>
rjo: parentheses :)
<whitequark>
your code has a bug.
<rjo>
you could even add sugar to support that without z: Match(s, "01-") would generate the expression.
<rjo>
whitequark: granted.
<whitequark>
also, if you change the bit masks, it is very easy for them to desynchronize.
<whitequark>
sugar could work, but it would also lower to exactly same or more complex logic.
<whitequark>
and it would still be order-dependent.
<whitequark>
(because of If/Elif)
<whitequark>
so I think there's no difference.
<rjo>
parentheses? where?
<whitequark>
m.Elif(s & 0b110 == 0b010)
<whitequark>
should be
<whitequark>
m.Elif((s & 0b110) == 0b010)
<rjo>
not in python afaict.
<whitequark>
mm
<whitequark>
let me recheck.
<whitequark>
ok, you are right.
<rjo>
and with don't cares, the expression is always the same, it doesn't get more complicated.
<whitequark>
what if you have a tree of don't cares?
<rjo>
i was confused because i had added such a decoder to misoc at some point and wasn't annoyed by the syntax.
<whitequark>
i think there's a few ways to do the same thing here.
<whitequark>
don't cares are used internally for If/Elif so exposing them is free.
<whitequark>
but you could do it another way, with Match.
<rjo>
that trett is just s & 0b11111000000 = 0b00001000000 etc.
<rjo>
tree
<whitequark>
yes.
<whitequark>
and then you lose four hours to miscounting one zero.
<whitequark>
i definitely have.
<whitequark>
multiple times just when implementing that core.
<whitequark>
when you give Case a bit string it errors out if length doesn't match so you can't have that.
<rjo>
i see that the notation is nice. i think that Match() would cut it. and it would prevent you from counting zeros and ones.
<whitequark>
sure.
<whitequark>
but we already have that notation without Match.
<whitequark>
why not use it?
<whitequark>
it also results in *much* nicer Verilog than If/Elif trees.
<rjo>
it's more implementation in the simulator, interfaces to external code, etc, no?
<whitequark>
nope. needed anyway.
<whitequark>
since If/Elif desugars to that
<rjo>
oh. you always need to rewrite if/elif into casez to satisfy rtlil?
<whitequark>
yes.
<rjo>
hmm. ok. but what if some verilog synthesizer asic tool chokes on z? wouldn't you prefer bit logic then?
<whitequark>
that seems extremely unlikely, as all proprietary tools handle z far better than yosys
<whitequark>
synopsys allows z internally in the fpga anywhere, for example
<whitequark>
but sure. in that case. what i do is tell yosys `proc` and it turns that directly into muxes.
<whitequark>
the reason i don't do `proc` right away is that the resulting verilog is less readable for no good reason.
<rjo>
and TBH readability of that casez with all branch conditions evaluated early and promoted to the top in rtlil isn't great.
<whitequark>
no. but it is better than the alternative
<whitequark>
i think i can teach write_verilog to be smarter about this.
<whitequark>
wait.
<whitequark>
promoted to the top/
<rjo>
for an IR it's fine i guess.
<whitequark>
*?
<whitequark>
are you looking at README examples or running yosys yourself?
<rjo>
i am looking at readme.
<whitequark>
ah ok.
<rjo>
and yosys decides when to convert a casez into if/else for verilog output (see arst.v)?
<whitequark>
arst is special.
<whitequark>
as in, asynchronous reset is detected with a special pass.
<whitequark>
everything else becomes case or casez. possibly always casez.
<whitequark>
this can definitely be improved on yosys side with only minor effort.
<rjo>
hmm. and do you expect z to be only available in Case? i forgot whether we discussed that aspect already.
<whitequark>
we did.
<rjo>
because as an address decoder for a bus that Match() would actually be nice.
<rjo>
that's outside of Case/If/Elif
<whitequark>
I think that can be arranged.
<rjo>
whether that Match internally uses z or bit logic doesn't matter to me. but the problems that we have had with counting zeros and ones appear in several places.
<whitequark>
yes. I agree that Match would be useful.
<rjo>
i would just shy away from using z in Signals or anything more complicated than that exact Match(expr, "01-")
<whitequark>
yes. it's not a real z anyway.
<rjo>
with that a record-destructuring match is also natural: Match(rec.field, "01-"). no magic needed.
<rjo>
and you prefer - over z in the syntax?
<whitequark>
definitely. z is already too overloaded in verilog.
<rjo>
ack.
<rjo>
why does m.Case(self.s, "01-") always repead the "self.s, "? can i switch selectors between branches?
<rjo>
*repeat
<whitequark>
to handle the case where you have two Cases on a different test one after the other
<whitequark>
you could do some magic to detect if the 1st argument is a Signal or not
<whitequark>
but this seems less prone to copy-paste errors
<rjo>
oh. the signal is the handle.
<whitequark>
yes.
<rjo>
hmm. isn't that prone to errors? with m.Case(a..): ... with m.Case(b..): ..., with m.Case(a...): ... would be three case blocks?
<rjo>
no error no warning?
<rjo>
same for m.Case(s + 1); m.Case(s) ?
<whitequark>
good point.
<whitequark>
suggestions?
<rjo>
or even the likely beginner error: m.Case(s[0]); m.Case(s[1])
<whitequark>
yes. definitely good points.
<rjo>
the only suggestion i can come up with quickly is to do the same as for FSM, add another indentation block.
<whitequark>
with m.Switch(s): with m.Case("1") ?
<whitequark>
ok.
rohitksingh has quit [Ping timeout: 240 seconds]
<rjo>
that aligns Case and FSM much more. at the expense of the (to me at least) awkwards C case double indentation syntax.
<whitequark>
sure.
<whitequark>
it is ... acceptable.
<whitequark>
rjo:
<whitequark>
examples/_cpu.py:93: DeprecationWarning: instead of `self.comb +=`, use `m.d.comb +=`
<whitequark>
self.comb += d.eq(v)
<rjo>
oh. maybe: with m.Case(s).Match("01-") as block: .... with block.Match("1--"): ... with block.Match(...)
<rjo>
ah. no.
<rjo>
doesn't help
<rjo>
whitequark: nice. thanks for that fallback!
<whitequark>
that's with `from nmigen.compat import *`.
<whitequark>
doesn't work otherwise.
<rjo>
good
<rjo>
about specials.
<rjo>
they are not specials anymore, but Modules now? the platform still knows how to lower them (memory, tristate, ddr) to primitives?
<whitequark>
Memory is now a FHDL primitive
<whitequark>
(there's no reason to not do that, Memory is quite magical anyway)
<rjo>
sounds right.
<whitequark>
things like tristate:
<whitequark>
you ask platform for a module that (presumably) wraps the right primitive for that platform.
<whitequark>
with plain old python like `platform.get_tristate()`
<rjo>
nit: the get_tristate() TSTriple mnemonics get me each time. I never know who to get() what from.
<whitequark>
we can fix that, sure.
<whitequark>
it annoys me too.
<rjo>
and when you ask the platform for a tristate, you get what? a module? a fragment?
<whitequark>
a module, I think, so you can stash it into submodules.
<rjo>
a module i presume.
<rjo>
ok. excellent that that lowering happens early (at the time the logic is generated).
<whitequark>
yes.
<_whitenotifier>
[m-labs/nmigen] whitequark pushed 3 commits to master [+5/-0/±5] https://git.io/fp5jV
<bb-m-labs>
build #355 of migen is complete: Failure [failed python_unittest] Build details are at http://buildbot.m-labs.hk/builders/migen/builds/355 blamelist: Pierre-Olivier Vauboin <povauboin@gmail.com>