[Libre-soc-bugs] [Bug 731] potential design oversight in Partitioned SimdSignal Cat/Assign/etc lhs/rhs

[bugzilla-daemon at libre-soc.org]

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

--- Comment #10 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Jacob Lifshay from comment #8)
> (In reply to Luke Kenneth Casson Leighton from comment #0)
> > Strategy / Incremental work plan:
> 
> reordering a bit for clarity:
> 
> > 2) incremental step of adding "mirror (trmporary) storage classes"
> 
> SwizzledSimdValue *is* that temporary storage class, it is just a little
> different than what you envisioned here:

unfortunately, it is much more than that.  it does that job *and* replaces
other code which has unit tests done *and* is a regression on capability
of exiating code *and* is not a drop-in replacement for existing code.

it is also an optimisation and it goes too far.

we absolutely have to be extremely strict about this and keep to
an incremental development strategy at all times.

if that class is to be used it needs to be scheduled as its own
incremental update with full unit tests and full functionality and
redesigned as a dropin replacement.

given that the existing classes are functional this firmly places
it into "not essential right now and in fact detrimental to urgent
timescales" territory.


> * use as rhs: its way of knowing when it's being used as the rhs is by
> trapping other code's accesses of SwizzledSimdValue.sig, and lazily adding
> the submodule that computes .sig's value. This way, no other code needs to
> operate differently when using SwizzledSimdValue as a rhs, it looks like a
> completely normal SimdSignal.
> * use as lhs: it overrides __Assign__ so doesn't use PartitionedAssign,
> instead returning a list of nmigen Assigns, therefore PartitionedAssign
> doesn't need to be aware of SwizzledSimdValue at all.

if you had separated that out as an incremental update it could have
been condidered.

> 
> > 
> > 1) Unit test showing and confirming need for this work        TODO
> >   a) first prototype
> >   b) Cat     TODO
> >   c) Part     TODO
> 
> iirc we weren't planning on supporting Part(...).eq(...), since, like Array,
> it's very complex and not super necessary.

i actually use that in dcache.py (!) but that is not ALU related (thank
goodness).

i realised retrospectively that i had looked at Value.__getitem__
and because of its int/Const checking assumed that Part offset must
also be int/Const.

again retrospectively i realised that Slice is const but Part is not,
so yes, Part is firmly off the table.

RHS style SimdSignal.__Part__ can be implemented in terms of a straight shift
and then slice.  it is about 3 lines of code due to SimdSignal shift being
fully
functional already.  depends on RHS SimdSignal.__Slice__ being done first
though.  discuss in bug #716.

> If we need Part, we can have a rhs-only class (like
> PartitionedDynamicShift), which should be good enough.

yes.  3 lines.

> lhs Part is hard, 

yep, let's skip it.
> >   d) Mux?     TODO
> 
> Mux(...).eq(...) doesn't work in nmigen, so we can remove that one from the
> list:

gone.

> >   e) Slice     TODO
> > 3) Add recognition (isinstance) for mirrors in PartitionedAssign  TODO
> 
> unnecessary, as outlined above

version2.  we are not doing version2.  or any kind of premature optimisation.
sorry.  this is basic Project Management.

> > 4) swap over to mirror classes in partsig.py one at a time
> >   a) Cat    TODO
> >   b) Part   TODO
> >   c) Mux?   TODO
> >   d) Slice  TODO
> > 5) create entirely new LHS Partitioned classes (or adapt existing ones)
> 
> We won't actually need any new classes, since SwizzledSimdValue is general
> enough that Cat, Slice, and Repl *could* be implemented entirely

again: sorry. basic Stable Project Management rules apply.  SwizzledSimdValue
is too far away from existing code. it is version2 and we have made it
clear that version 2 is off the table until version 1 is complete.


> > note that the "mirror" classes for Cat, Part etc do not actually
> > do anything, they act as simply temporary storage for the arguments
> > passed to ast.Cat() etc pending encountering an Assign, at which
> > point and ONLY at which point the missing information (LHS or
> > RHS) is apparent.
> 
> That works great for LHS, however, for RHS use, you need the actual bits of
> a Cat, Slice, etc. SimdSignal well before encountering a .eq() call.
> SwizzledSimdValue handles that by trapping self.sig and lazily computing its
> value if needed.

if you had followed incremental development practices essential for a
project of this size and separated out the concepts i would be able to
follow this.

i don't understand or follow how a simple substitution of classes which
defer creation of classes that use the parameters could
cause RHS to stop working.

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