<mwk>
whitequark: how do you test the cxxrtl backend? I'll need to add the new FF types for https://github.com/YosysHQ/yosys/pull/1818 there and would rather not break stuff...
<whitequark>
mwk: unfortunately, manually
<whitequark>
what i was planning to do at some point is hooking it up to vloghammer
<whitequark>
but i don't think vloghammer even produces those FF types
<whitequark>
hang on
<whitequark>
why do you have $dffs with RST input but $dlatchr?
<daveshah>
s=synchronous
<whitequark>
well that's not confusing at all
<whitequark>
shouldn't it be $sdff, like $adff?
anticw is now known as cw
cw is now known as Guest3061
Guest3061 is now known as anticw
<whitequark>
mwk: in general, adding new FF types won't break cxxrtl as it will simply assert on them as on any unknown cell type
<whitequark>
implementing $dffsre, $adffe and $dlatchr is a matter of adding those cells to the whitelist
<whitequark>
the sync DFF cells will need to be updated separately
<mwk>
whitequark: they were already called dffs in the dff2dffs pass
<mwk>
also, this way I can differentiate ce vs rst priority by calling them dffse / dffes
<whitequark>
this kind of naming *will* lead to bugs
<mwk>
I agree, the naming is horrible, and I'm open to improvement suggestions, but renaming the existing cells (in particular adff) is ill-advised
<whitequark>
but whatever
<daveshah>
That was for the gate level cell
<daveshah>
Which have conventionally always put stuff after DFF
<whitequark>
hm, i wasn't suggesting renaming adff
<daveshah>
I don't think renaming dff2dffs would be a major problem
<mwk>
daveshah: the gate level cells are horrible by not even differentiating between dff vs adff, they're both called $_DFF_*_, just with more letters
<daveshah>
Indeed
<mwk>
sure, dff2dffs is getting renamed anyway by virtue of promotion to actual builtin cell types
<mwk>
just... give me a consistent set of cell names
<daveshah>
Let's say adff and sdff
<mwk>
in particular the rst vs ce priority is annoyging
<daveshah>
Nothing else changes
<mwk>
alright, what about sdffe?
<daveshah>
Oh, that's a problem
<whitequark>
i think the $sdffe where the enable has priority over reset should be the one changed
<whitequark>
since in all other cells, reset has priority over all other controls
<mwk>
fine, changed to what?
<daveshah>
Yes, and it's the less common case
<daveshah>
sdffep for 'enable priority'
<daveshah>
Or is that too confusing with p for positive
<whitequark>
I think $sdffep is ok
<whitequark>
in all those cells, positive/negative is set via parameters
<whitequark>
it's not ideal, but it's not severely misleading either
<whitequark>
alternatively
<whitequark>
$sdffc where c stands for "control enable"
<whitequark>
since it, well, enables control inputs
<mwk>
then we'll end up with $_SDFFEP_NN0N_" or something
<whitequark>
ok, didn't realize that
<whitequark>
what do you think about $sdffc?
<mwk>
kind of strange, but fine with me
<whitequark>
a bit strange to me too, but it seems like an okay way to handle a relatively uncommon cell
<mwk>
alright, that'll be $dffs, $dffse, $dffes renamed
<mwk>
$adffe, $dffsre are obvious, I guess
<mwk>
any complaints about $dlatchr?
<mwk>
not particularly happy about it, but...
<whitequark>
would it be more consistently named $adlatch?
<whitequark>
I'm not entirely clear on the semntics
<mwk>
yeah, it would
<mwk>
but uhh "asynchronous d latch"
<whitequark>
I agree, but $dffsr is already asynchronous
<whitequark>
so you're not making the scheme worse
<whitequark>
you're making it... exactly as inconsistent as it was before
<mwk>
a noble goal
<whitequark>
unsatisfying, but better than alternatives, hopefully?
<mwk>
fair enough
<mwk>
I guess I should also rename the control signal from RST to ARST for consistency?
<mwk>
and perhaps the $sdff's RST -> SRST?
<mwk>
also, how about $sdffce
<mwk>
the lone 'c' looks too similar to 'e' IMO
<whitequark>
yes to all three
<mwk>
alright
<whitequark>
that will also make fixing cxxrtl (and presumably other code) a lot easier
<whitequark>
since cxxrtl uses a combintorial kinda approach to these cell
<whitequark>
matching them with a whitelist and then looking at which ports exist
<mwk>
yeah, there are more places like that
<whitequark>
also, wth happened with the manual in your PR?
<mwk>
?
<whitequark>
oh nevermind, I misread something in the diff
<mwk>
it... uhhh kind of ran out of the page
<mwk>
so I ended up splitting the big table of tables into several tables
<mwk>
because there was no way the combined thing would fit in a single page anymore
<whitequark>
yeah I figured this out now
<whitequark>
I thought there was a generator issue or something at first
<whitequark>
I'm very happy to see this PR by the way, thank you for working on this
<whitequark>
I don't think I'll directly benefit from it but it seems like a longstanding omission
<mwk>
yeah
<mwk>
the functionality just ended up duplicated everywhere
<mwk>
... this finally happened when I vetoed https://github.com/YosysHQ/yosys/pull/1784 for having *another* ad-hoc sync reset + clock enable detection in pmgen (I hate pmgen by the way)
<mwk>
so now I'm of course supposed to do it Properly
<whitequark>
btw, have you seen my BRAM inference attribute PR?
<mwk>
the ramtype thing that you merged recently?
<mwk>
I haven't had the time to review it properly, unfortunately
<whitequark>
yes, that one
<whitequark>
for the most part I didn
<whitequark>
't request your review (though it is of course welcome) but wanted to give heads up
<whitequark>
since Xilinx also implements those but I'm not a Xilinx expert
<mwk>
tbh I consider blockram inference to be a disaster area anyway, to be entirely rewritten
<whitequark>
my PR is orthogonal to that
<whitequark>
it just makes it so that there is a way to say "this gotta be a block RAM, fail the synthesis if you cn't infer one"
<whitequark>
instead of exploding it into forty gigabytes of flip flops
<whitequark>
and probably crashing your machine through memory pressure
<mwk>
right, looks like a good idea
<whitequark>
in general, I think nmigen should be altered to pass `syn_ramstyle=auto` on ice40 and ecp5 now
<whitequark>
to make sure Memory ends up as BRAM or LUTRAM
<mwk>
... I suppose it'd be good to have some attribute always recognized by all yosys flows? I mean, consistency with vendor toolchains is good, but easy porting from one yosys-supported FPGA to another would be even better
<whitequark>
there is one
<daveshah>
Particularly for iCE40 very small memories don't make sense as BRAM
<whitequark>
ram_block, rom_block, logic_block are all specified by 1364.1
<daveshah>
as there's no distributed RAM
<mwk>
ohh, I didn't notice that
<mwk>
nice
<daveshah>
ie less than 64 bits, particularly ROMs
<whitequark>
daveshah: hm, point
<whitequark>
I might have to implement something more complex here
<mwk>
hmm
<mwk>
fun fact: xilinx actually has special "distributed ROM" primitives
<mwk>
... yes, they are identical to LUTs
<whitequark>
that seems fine to me
<whitequark>
ecp5 also has those
<whitequark>
i think, at least? i've seen it in the toolchain docs
<mwk>
the one major difference is that DRC doesn't yell at you if there's a useless address bit
<mwk>
I wonder what memory_map does for RO memories
<mwk>
perhaps it'd actually be worth it to support these primitives
<mwk>
(that or make it $lut-aware, which could be close enough)
<daveshah>
No because then they can't be optimised
<daveshah>
By converting to logic it means more optimisation possible which is often the case for small, non randomly filled ROMs
<whitequark>
oh
<whitequark>
is this how xilinx optimizes out unused instructions from soft CPUs with embedded ROM?
<mwk>
one thing I'd definitely like to see some day is a way to mark ROM contents as "changeable post-P&R", which would make yosys use the distributed ROM primitives, and have some "replace ROM data" program you can run between p&r and bitgen
<daveshah>
Yes, that sounds like a good idea
<mwk>
but uhhh, probably let's make ram inference less of a superfund site first
<whitequark>
yeah
* mwk
wonders what messy PR will she end up vetoing next to end up in charge of rewriting memory inference
<mwk>
because let's face it, that's going to happen some day
* whitequark
looks up
<whitequark>
ECP5 has syn_romstyle=logic
<whitequark>
yup, and ROM16X1 and so on
<daveshah>
I have a feeling that it uses the horrible number in string thing
<daveshah>
Actually looks OK
<daveshah>
It's only the RAM that uses numbers in string
<mwk>
the wha... ok I don't want to know, do I
<daveshah>
C integer literals in a Verilog string
<daveshah>
Radiant uses these for everything
<mwk>
marvelous
<daveshah>
Diamond and iCEcube only use these for some things
<whitequark>
you know what i hate? syn_ramstyle=block, but syn_romstyle=ebr
<_whitenotifier-3>
[whitequark/Boneless-CPU] whitequark pushed 27 commits to master [+29/-21/±128] https://git.io/Jvhbr
<_whitenotifier-3>
[whitequark/Boneless-CPU] tpwrules 6ae9127 - doc: add syntax definition for operation sections in preparation for making the sections follow it
<_whitenotifier-3>
[whitequark/Boneless-CPU] tpwrules c61e7fe - doc: rewrite subtraction as 2s complement addition because it matches the hardware
<_whitenotifier-3>
[whitequark/Boneless-CPU] tpwrules 44f6da0 - doc: correct carry/borrow bit generation for subtractiony instructions
<_whitenotifier-3>
[whitequark/Boneless-CPU] ... and 24 more commits.