[libre-riscv-dev] [Bug 318] fix LDSTCompUnit

[bugzilla-daemon at libre-soc.org]

https://bugs.libre-soc.org/show_bug.cgi?id=318

--- Comment #20 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Cesar Strauss from comment #19)
> (In reply to Luke Kenneth Casson Leighton from comment #18)
> > Cesar, fyi:
> > 
> > https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=c501606b20c74050fb0d422ff886a7d9fb2a6a96
> > 
> > very straightforward, no disruption at all, due to a hack
> > of making MultiCompUnit look exactly like it was.
> 
> Dear Luke,
> 
> I see you just committed:
> https://git.libre-soc.org/?p=soc.git;a=commitdiff;
> h=1dbb3d17d051f73a20a148740be7b65c1a1778c1

yes.  i meant to write here this morning, but got side-tracked on pipeline
code-morphing.

> I had the impression, maybe wrongly, that you had assigned this task to me.

ah.  sorry!  if you had written explicitly "i will do this conversion tomorrow 
evening", i would have held off.

> I had my own version mostly working. I will compare mine to yours, and make
> a review of yours.

appreciated.

> I guess I failed to explicitly accept the (supposed) assignment, and report
> my progress. Sorry about that. Lesson learned.

by both of us.

> Too bad about the duplicate effort. No worries. At least I had a good
> training on nMigen, RecordObject and the CompUnit API.

about the CompUnit API: if you look at CompUnitBase you can see where i'm
going with it.
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/experiment/score6600_multi.py;h=4630368e474c9d7532dad3b00a3983435ba01c7b;hb=7b0b09aef28e4329c6b85d933b022112f0c15c50#l44

CompUnitBase is a mess.  assigning individual signals from a batch of CompUnits
to create a common data structure.   i am hoping - planning - to find a way
to merge the units into a common data structure.  or, if not, at the very
least a common *python* level API which allows convenient signal-connectivity
without having a hundred lines of tediously repetitive signal-for-signal 
assignments.



about RecordObject: RecordObject exists because whitequark wants Record to be
bit-oriented, particularly when it comes to enumerating (using __iter__)
through it.

whereas what we need - to avoid insanely complex graphviz diagrams that are
flat-out impossible to read - is *field*-orientated iteration and assignment.

*field*-orientated iteration allows nmigen to generate code that, when you
assign one RecordObject to another, does so by creating eq()s on the *members*
of the RecordObject, not the *individual bits of every single field*.

also, RecordObject looks pretty much exactly like a standard python class,
and may be sub-classed (as you can see from LDSTCompUnitRecord) and extended
in a standard python fashion.

Record (because it takes an explicit layout argument) may *NOT* be extended
except by direct manipulation of Record.fields, which can have consequences
when people assume that the use of any given Record instance will have exactly
the same structure.

basically, RecordObject fits python developer expectations: Record does not.


> On the way, I did manage to catch an obvious typo in your previous
> refactoring of compalu_multi.py. I took the liberty to commit it:
> 
> https://git.libre-soc.org/?p=soc.git;a=commitdiff;
> h=d9724d6fed90959a78799c6e48667d31abd3a091 

yes i saw - that would have been hard to track down: the unit test didn't
catch it.  really appreciated.

> I'm going back now to improve the unit test.

ok :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.