(In reply to Jacob Lifshay from comment #0)
> I discovered this by running:
> sv.maddedu *4, *32, 36, 8
> and was confused by why it kept reading RB from register 68 not 36

now that I think about a bit more, PowerDecoder2 must be incorrect, since r68 shouldn't even be encodeable.

scalar you know that it is r0-r63 as specified many years ago.

I added some tests to see if the assembler(s) match the simulator.

I then fixed PowerDecoder2 and the algorithms on the wiki. now the 256-bit mul and the new tests I just added pass!

If someone can double check the changes to the wiki, I'd greatly appreciate that -- that's all that's left for this bug

https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=24576f370d5b0b0282b821062c66e1ff39ab8019

commit 24576f370d5b0b0282b821062c66e1ff39ab8019
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Fri Sep 15 14:47:00 2023 -0700

    fix scalar EXTRA2 in EXTRA2/3 decoding algorithms

https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=630dfa6c8b6633d66d1a41368dfad927754846ed

commit 630dfa6c8b6633d66d1a41368dfad927754846ed
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Fri Sep 15 14:21:00 2023 -0700

    fix PowerDecoder2 to properly decode scalar EXTRA2
    
    https://bugs.libre-soc.org/show_bug.cgi?id=1161

commit cce08ed213d1de4d2f11b493917e1c34f9c40b61
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Fri Sep 15 14:20:09 2023 -0700

    add tests for checking if the simulator and assembler agree on SVP64 encodings

(In reply to Jacob Lifshay from comment #3)

> If someone can double check the changes to the wiki, I'd greatly appreciate
> that -- that's all that's left for this bug

Jacob you are not authorized to make arbitrary specification changes without
FULL consultation and review. you have no idea what the consequences are,
and in the unauthorized change that you made you missed a number of places
where EXTRA2/3 is specified.

do not ever do that again.

please create a branch for specification changes, discuss the proposed
modifications on the bugtracker ONLY, and WAIT for them to be Authorized.

jacob i've now had the opportunity to think about this (which was
needed in my own time and *before* any modifications are made).

the use of the intermediate variable "extra" is there to make the
spec easier to read. please preserve it. use the following strategy:

if EXTRA3:  # EXTRA3 scalar is EEnnnnn vector is nnnnnEE
  extra = spec[1:2]
elif isvec: # EXTRA2 vector gives r0-126 in steps of 2 nnnnnE0
  extra = spec[2] << 1
else        # EXTRA2 scalar gives range r0-r63 0Ennnnnn
  extra = spec[2]

# now with the difference between EXTRA2 and EXTRA3 removed
# all that is left is to detect vec/scalar
if isvec: SVregnum = regnum<<2 | extra
else      SVregnum = regnum    | extra<<5

this will need adapting for CR-with-SV.

you will need to find *all* locations where the above algorithm
needs to be placed. please find them and put the corrections into
a branch, and also update the PowerDecoder2 variant to match the
above so that anyone reading the spec and the source code can
see clearly that they are related.

an additional unit test suite is needed which puts register values in
to high numbers then say "adds one" to each (sv.addi r80, r80,1) with
say VL=4, and checks that they all are correctly updated.

a set of instructions will have to be selected which cover all 5
categories: NORMAL CROPS BRANCH LDST_IMM LDST_IDX

this will need to be ISACaller/HDL unit tests rather than asm/disasm
tests of binutils.

(In reply to Luke Kenneth Casson Leighton from comment #6)
> an additional unit test suite is needed which puts register values in
> to high numbers then say "adds one" to each (sv.addi r80, r80,1) with
> say VL=4, and checks that they all are correctly updated.

ok, that should be doable...

> a set of instructions will have to be selected which cover all 5
> categories: NORMAL CROPS BRANCH LDST_IMM LDST_IDX

we'll probably also want FP insns. we'll need a representative example instruction for each category, that insn also needs to have the properties necessary for easy testing of all input/output register fields (hence why I picked maddedu rather than isel, since isel doesn't copy the condition input to any output (so we can check that the input pattern is in the output reg), making it easier to end up with a unit test that passes even though the insn is broken.)
> 
> this will need to be ISACaller/HDL unit tests rather than asm/disasm
> tests of binutils.

the unit tests I already wrote should do that, they just need to be extended to the additional insns and VL=4.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/test/svp64/encodings.py;h=d5bf476c7fd036889767d0d140afa3030f908c48;hb=cce08ed213d1de4d2f11b493917e1c34f9c40b61

important context and discussion for time/budget/workload assessments:

https://lists.libre-soc.org/pipermail/libre-soc-dev/2023-September/005646.html

(In reply to Luke Kenneth Casson Leighton from comment #5)
> the use of the intermediate variable "extra" is there to make the
> spec easier to read. please preserve it. use the following strategy:

ok, I'm assuming you mean `spec`.

I changed the code to:
```
    if extra3_mode:  # EXTRA3 scalar is EEnnnnn vector is nnnnnEE
        spec = EXTRA3
    elif EXTRA2[0]:  # EXTRA2 vector gives r0-126 in steps of 2 nnnnnE0
        spec = EXTRA2 || 0b0
    else:            # EXTRA2 scalar gives range r0-r63 0Ennnnn
        spec = EXTRA2[0] || 0b0 || EXTRA2[1]
    if spec[0]: # vector
        return RA || spec[1:2]
    else:       # scalar
        return spec[1:2] || RA
```

what do you think? I'll update the rest after getting your ok.

https://git.libre-soc.org/?p=libreriscv.git;a=blob;f=openpower/sv/svp64.mdwn;h=f5b9e9ef340a6861630deb8848ac5ef72f6793fc;hb=ac4a9288f342e451fa4dbcab7db3321e8a14d9c7#l1192

now that I'm looking a bit more, I think we should use the syntax used by PowerISA's pseudocode, so it needs a bit more adjustment.

(In reply to Jacob Lifshay from comment #9)
> 
> what do you think? I'll update the rest after getting your ok.

Please confirm you have checked Jacob's work and dis/approve.

> 
> https://git.libre-soc.org/?p=libreriscv.git;a=blob;f=openpower/sv/svp64.mdwn;
> h=f5b9e9ef340a6861630deb8848ac5ef72f6793fc;
> hb=ac4a9288f342e451fa4dbcab7db3321e8a14d9c7#l1192
> 
> now that I'm looking a bit more, I think we should use the syntax used by
> PowerISA's pseudocode, so it needs a bit more adjustment.

Although we want to be more aligned with the PowerISA spec, let's keep priorities straight.

Right now, this task *must* be completed first (to make sure spec and simulator are sync'd up) with the current pseudocode syntax (make no other modifications). In the future (probably in a new grant), we can allocate budget for tasks related to making our spec more consistent with OPF's.

(In reply to Jacob Lifshay from comment #9)
> (In reply to Luke Kenneth Casson Leighton from comment #5)
> > the use of the intermediate variable "extra" is there to make the
> > spec easier to read. please preserve it. use the following strategy:
> 
> ok, I'm assuming you mean `spec`.

no i said the extra variable, as it was before you made changes

> I changed the code to:
> ```
>     if extra3_mode:  # EXTRA3 scalar is EEnnnnn vector is nnnnnEE
>         spec = EXTRA3

extra =

>     elif EXTRA2[0]:  # EXTRA2 vector gives r0-126 in steps of 2 nnnnnE0
>         spec = EXTRA2 || 0b0

extra =

>     else:            # EXTRA2 scalar gives range r0-r63 0Ennnnn
>         spec = EXTRA2[0] || 0b0 || EXTRA2[1]

(In reply to Luke Kenneth Casson Leighton from comment #12)
> (In reply to Jacob Lifshay from comment #9)
> > (In reply to Luke Kenneth Casson Leighton from comment #5)
> > > the use of the intermediate variable "extra" is there to make the
> > > spec easier to read. please preserve it. use the following strategy:
> > 
> > ok, I'm assuming you mean `spec`.
> 
> no i said the extra variable, as it was before you made changes

it was spec before i made changes, i had changed it to extra (in the CR pseudocode), but that was reverted. if you're ok with having the name changed from what it was, i'd be fine changing it to extra.

(In reply to Jacob Lifshay from comment #13)

since I'm waiting for luke to reply, I went ahead and created
a `fix-scalar-extra2` branch for the corresponding openpower-isa changes:

https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=refs/heads/fix-scalar-extra2

https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=a723e3c7726241dc2b505b66a43932b841696dbc

commit a723e3c7726241dc2b505b66a43932b841696dbc
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Sep 19 18:36:32 2023 -0700

    implement the algorithm from the wiki basically verbatim

commit 792d0f42f8d95a122cb334365e1bf347d92aeafa
Author: Jacob Lifshay <programmerjake@gmail.com>
Date:   Tue Sep 19 18:35:22 2023 -0700

    format code

sorry been too long since i looked at this.  i thought it was a
variable named "extra" not "spec".

original:

    if extra3_mode:
        spec = EXTRA3
    else:
        spec = EXTRA2<<1 | 0b0
    if spec[0]:
       # vector constructs "BA[0:2] spec[1:2] 00 BA[3:4]"
       return ((BA >> 2)<<6) | # hi 3 bits shifted up
              (spec[1:2]<<4) | # to make room for these
              (BA & 0b11)      # CR_bit on the end
    else:
       # scalar constructs "00 spec[1:2] BA[0:4]"
       return (spec[1:2] << 5) | BA

insert correction here:

    if extra3_mode:
        spec = EXTRA3
=>> elif  <===
    else:
        spec = EXTRA2<<1 | 0b0

two pages,

https://libre-soc.org/openpower/sv/svp64/
https://libre-soc.org/openpower/sv/svp64/appendix


i need you to keep diffs to the absolute minimum.

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=a723e3c7726241dc2b505b66a43932b841696dbc

please revert ALL changes that you made, in the branch (do not worry
about tests failing), start again including on PowerDecoder2,
and make ONLY changes that keep
to the absolute bare minimum of CLEAR diff changes.

that way i can easily review them.

(In reply to Luke Kenneth Casson Leighton from comment #15)
> https://libre-soc.org/openpower/sv/svp64/
> https://libre-soc.org/openpower/sv/svp64/appendix

ok, I changed both the INT/FP and CR EXTRA2 algorithms:

https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=7a232bcca2fafd3696ab01598ec9aac42d7db825

I reverted all recent changes to power_svp64_extra.py and made minimal changes that make tests pass and match the wiki commit above:

https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=3a7959910dd014c68427bb80b58bbd5e09e4d9e8

(In reply to Jacob Lifshay from comment #16)
> (In reply to Luke Kenneth Casson Leighton from comment #15)
> > https://libre-soc.org/openpower/sv/svp64/
> > https://libre-soc.org/openpower/sv/svp64/appendix
> 
> ok, I changed both the INT/FP and CR EXTRA2 algorithms:
> 
> https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;
> h=7a232bcca2fafd3696ab01598ec9aac42d7db825

ahh that's more like it. coment could do with being

 # vector mode r0-r126 increments of 2 (something like that)
 # scalar mode r0-r63

> I reverted all recent changes to power_svp64_extra.py and made minimal
> changes that make tests pass and match the wiki commit above:
> 
> https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;
> h=3a7959910dd014c68427bb80b58bbd5e09e4d9e8

brilliant, that's really clear. the introduction of extra2_lsb
makes it easy to know what is happening. and fixes the bug. great work.

ok i am happy with this, feel free to rebase.
and let's transfer over the budget from bug #1083

(In reply to Luke Kenneth Casson Leighton from comment #17)
> ahh that's more like it. coment could do with being
> 
>  # vector mode r0-r126 increments of 2 (something like that)
>  # scalar mode r0-r63

I added comments to that effect to power_svp64_extra.py and to svp64.mdwn

I rebased on master and pushed to both master and fix-scalar-extra2:

https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=be96bff2a401920cac82dedbc1f7a1c27345e25d

https://git.libre-soc.org/?p=libreriscv.git;a=commitdiff;h=dea437afb25dbf082532a39f8e2d122d079e6b10

> 
> ok i am happy with this, feel free to rebase.
> and let's transfer over the budget from bug #1083

looks like you already did that, thx!

(In reply to Jacob Lifshay from comment #18)

> 
> I rebased on master and pushed to both master and fix-scalar-extra2:

fantastic.

> looks like you already did that, thx!

i did but i realised after that EXTRA322 has to be done, as libopid
funding of future is not immediate. i will sort somewhere else to
put this task.

(In reply to Luke Kenneth Casson Leighton from comment #17)
> ok i am happy with this, feel free to rebase.
> and let's transfer over the budget from bug #1083

The parent task assigned is bug #1003, *not* bug #1083.
Was this intended?

(In reply to Andrey Miroshnikov from comment #20)
> (In reply to Luke Kenneth Casson Leighton from comment #17)
> > ok i am happy with this, feel free to rebase.
> > and let's transfer over the budget from bug #1083
> 
> The parent task assigned is bug #1003, *not* bug #1083.
> Was this intended?

no. i corrected it (just have to doublecheck running budgetsync)
i have added it here https://bugs.libre-soc.org/show_bug.cgi?id=1056#c0
it is NOT listed as a subtask, just increased jacon's share of bug #1056
so that the MOU does NOT need changing/updating with NLnet.