[Libre-soc-dev] unauthorized SVP64 specification changes

[Jacob Lifshay]

On Sun, Sep 17, 2023 at 11:50 PM Luke Kenneth Casson Leighton
<lkcl at lkcl.net> wrote:
> you missed for example that although you updated the pseudocode
> for CR EXTRA2/3 you did not do the corresponding GPR/FPR code
> in the same commit.

well, actually, it's kinda the other way around -- I updated the
pseudocode for GPR/FPR EXTRA2/3 to match the table since the table is
what both you and I agree is correct. While working on that, I noticed
that CR EXTRA2 was likewise inconsistent, so attempted to correct it.

>  i don't know exactly where it is, i just
> know it's there *and don't have time to find it*, that's down
> to you.

That's the first thing I updated (note the commit is the one you reverted):
https://git.libre-soc.org/?p=libreriscv.git;a=blob;f=openpower/sv/svp64.mdwn;h=8bc7f784b815497345306f1472781cda888a26f4;hb=24576f370d5b0b0282b821062c66e1ff39ab8019#l1196

>
> reminder: when making changes to the spec it is of ABSOLUTE TOP
> PRIORITY ABOVE ALL ELSE WITHOUT FAIL that they be correctly
> implemented IMMEDIATELY at the same time in ISACaller with
> NO OTHER DEPENDENT WORK BEING CARRIED OUT UNTIL ISACaller IS
> UPDATED, and unit tests created.
>
> (you already made the mistake of rushing ahead with the bigmul
> REMAP being hard-dependent on an Unauthorized modification of
> the specification, so you now know *why* there is the above
> hard rule).

Actually, I wrote the assembly before I realized there was a spec.
bug, so, no, it was not rushing ahead, it was discovering after the
fact that it depends on insndb and PowerDecoder2 agreeing on EXTRA2
encodings.

> that said the fact that you picked up the error is extremely
> important: the solution however may turn out to be more
> complex and more involved than the PRELIMINARY (Unauthorized)
> fix that you implemented, in order to save gates at the Decoder.

I agree we should look into it more, but I think the deciding factor
won't be how many gates it uses (I expect that difference to be on the
order of <0.1% -- a few gates), but instead on how easy it is to
understand.
>
> given that that involves huge modifications right down the chain
> of dependencies right the way through spec HDL ISACaller binutils
> it has to be properly evaluated for its time and cost to the
> project.

As far as I know, insndb is correct for FPR/GPRs (and probably CRs
too, but I'm less confident on that). I expect binutils to match
insndb.
>
> the good news is that you wrote unit tests for picking up
> the (interim, unapproved) fix.

yeah, basically I wanted that test to be comprehensive and cover all
SVP64 fields, but figured that is such a large amount of work that it
should be its own task, so just did tests of all EXTRA2/3 GPRs (from
r0 to r127) for 2in-1out (all of RT, RA, and RB) and 3in-1out (and
also RC) now.

> i have a suggestion there which
> i will put on the bugreport, for ISACaller / TestIssuer.

will respond on the bugtracker.

> yes i am aware that TestIssuer and ISACaller have limits on
> the range of CR Fields (max 8 when it should be 128), do take
> that into consideration when writing the tests (@"skip").

maybe we should add a test that just creates a PowerDecoder2 by itself
and just attempts to decode insns assembled by insndb? that way we can
test decoding CR fields above 8 without needing to modify the whole
simulator.

Jacob