[Libre-soc-dev] my current plan regarding simdsignal

[lkcl]

On Sun, Oct 24, 2021 at 6:48 AM Jacob Lifshay <programmerjake at gmail.com> wrote:

> I meant wrapping in this sense:
> # exactly how code would look in our .py files
> class MyModule(Elaboratable):
>     ...
>     def elaborate(self, platform):
>         m = Module()
>         with SimdScope(self, m):
>             # normal body
>             a = SimdSignal(5)
>             b = SimdSignal(5)

no. this is hidden state - hidden side-effects - which is extremely hard
for inexperienced programmers to follow and is yet another very bad
programming practice (on top of an already well-known bad practice
of using globals)

i have made it clear that the API has to be explicit, and have already
put it into the docstring for the module:
https://git.libre-soc.org/?p=ieee754fpu.git;a=blob;f=src/ieee754/part/simd_scope.py;hb=HEAD

  10 use as:
  11
  12     m = Module()
  13     with SimdScope(m, elwid) as s:
  14         a = s.Signal(width=64, ....)
  15
  16     m.d.comb += a.eq(...)
  17
  18 """

i have said THREE TIMES now that this is how the API was
always envisaged, from over 18 months ago, and how it is
going to be.

to make sure it's clear allow me to point it out in diff style
from the example that you give (which you keep repeating
and don't acknowledge, every time, even when i point out
that you don't acknowledge it, so this time could you please,
at last, acknowledge it):

-         with SimdScope(self, m) as s:
+        with SimdScope(self, m):
            # normal body
-             a = SimdSignal(5)
+             a = s.Signal(5)

ok?  this makes the use of hidden state (hidden side-effects) - which is
*another* extremely bad practice - unnecessary.

it's dead simple, and it's dead obvious that the type of Signal will
change as determined by the ContextManager, SimdScope.

it's easy to explain to inexperienced new programmers, and that's
really, really important.  it's also easy to read, and clear that there's
something to be expected to be slightly different from standard
nmigen usage.

people will then ask about that, and we can say, "those functions
s.Signal are to be regarded as exactly the same as nmigen
Signal, but they can be redirected to API-identical SIMD variants,
depending on context (which is why a ContextManager is used)
the ContextManager SimdScope takes care of that so you
don't have to be concerned that there's any major difference
in the nmigen API (because there isn't any)".

the plan that you have for this literally compounds two to three
different types of complexity on top of a chain of not one but
*two* separate and distinct types of bad programming practices!

by contrast, the plan i came up with over 18 months ago can be
implemented in a ridiculously small number of lines of code and
has none of the unintended side-effects or well-known bad
programming practices.

[side-note: the only reason why ISACaller has to use hidden state -
and is documented as best i can manage - is because frickin yield
won't let you do return results from functions, because the damn
yield item *is* the return result]

putting variables into hidden state that are picked up by the
parent is such bad practice and such an untested code-path
in python that i actually managed to cause python 3.7 to
segfault when trying it with yield.

no that's not "crash as in throw a python exception" which is what
most inexperienced python programmers mislabel a python
exception as: the actual c binary executable *actually* crashed with a
segfault and a coredump.

somewhere deep in python's code there are signs that the
refcounting is off.  print statements of some variables being
"returned" (via hidden state) showed their value to be *zero*
outside of the use of one yield (in the parent caller), where
inside - at the point where they were put into hidden state -
they were non-zero.

this should give you some indication of how little-used the
technique of hidden state really is

(!)

> That said, since you don't like globals, if we *instead* do:
> class SimdScope:
>     def __init__(self, ...):
>         ...
>         self.Signal = partial(SimdSignal, scope=self) # we need partialclass...
>         self.Shape = partial(SimdShape, scope=self)
>         self.Signal_like = partial(SimdSignal.like, scope=self)
>         ...

this is pretty much exactly what i've already added, line 141, it's
a little different (a wrapper, called partial, which i explain below
is a meta-technique i would prefer not be used)

 141     def Signal(self, shape=None, *, name=None, reset=0, reset_less=False,
 142                  attrs=None, decoder=None, src_loc_at=0):
 156             s = SimdSignal(mask=self, # should contain *all*
context needed,
 160                           shape=shape, # should contain the *secondary*
 164                           name=name, reset=reset,
 165                           reset_less=reset_less, attrs=attrs,
 166                           decoder=decoder, src_loc_at=src_loc_at)

[scope is passed in to SimdSignal via the variable named "mask".
which is a bit confusing but keeps the (old) PartitionedSignal API
around for now.  if the (old) API is ever deleted this parameter
can be changed]

although i'd only thought ahead as far as Signal, i realise now that
because Signal.like is a static function that you're right, Signal_like
will have to be added as well, and a SimdScope.Shape as well,
in order to allow "pick-up" of the scope and passing it on transparently.

i'd prefer that these were added in the same (explicit) style that
SimdScope.Signal is done in because i've worked with so many
hidden-attribute and dynamic node/getattr/setattr overriding
methods in my time, i know they always end up being almost
impossible to understand except by people with meta-programming
experience and skill.

in this particular instance, not using "partial(...)" provides an
opportunity to add extremely clear docstrings on something that
is a critical strategic part of what we're doing.

we need as many opportunities to explain this VERY CAREFULLY
to people (if that big EU Grant is accepted we'll have literally
20+ new programmers suddenly dropped in our lap, David and
I actually had to add in Training classes into the Budget)

a meta-programming technique such as wrapping with a function
called "partial()" actively hinders that opportunity.

i've just added them as TODOs:

https://git.libre-soc.org/?p=ieee754fpu.git;a=commitdiff;h=31610c7eac17e9e5c52e2a53d7cb6d5d744c7b07

and left room for explaining that other functions should be added as
well... argh, i can see that Value.cast is going to be needed there
as well, sigh.

l.