We have the code which manually establishes mappings between various fields: pysvp64asm, power_svp64_extra.py, power_svp64_prefix.py, power_svp64_rm.py, various decoders and so on. They all rely on consts.py; however, it's up to author to ensure which field goes where.

I realized that we have to provide some uniform solution so that it can be used by several parties. Especially since I want to re-use this uniform solution for binutils and generate as much code as possible.

Here are the new-style fields:

class RM(Fields):
    """SVP64 RM: https://libre-soc.org/openpower/sv/svp64/"""
    spr: Field = range(24)
    mmode: Field = (0,)
    mask: Field = range(1, 4)
    elwidth: Field = range(4, 6)
    ewsrc: Field = range(6, 8)
    subvl: Field = range(8, 10)
    extra: Field = range(10, 19)
    mode: Field = range(19, 24)
    extra2: Field[4] = (
        range(10, 12),
        range(12, 14),
        range(14, 16),
        range(16, 18),
    )
    smask: Field = range(16, 19)
    extra3: Field[3] = (
        range(10, 13),
        range(13, 16),
        range(16, 19),
    )


class Prefix(Fields):
    """SVP64 Prefix: https://libre-soc.org/openpower/sv/svp64/"""
    insn: Field = range(32)
    major: Field = range(0, 6)
    pid: Field = (7, 9)
    rm: RM = ((6, 8) + tuple(range(10, 32)))


for (name, fields) in {"RM": RM, "Prefix.rm": Prefix.rm}.items():
    print(name)
    for (key, value) in fields:
        print("    ", key, value)


The output is:

RM
     spr (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23)
     mmode (0,)
     mask (1, 2, 3)
     elwidth (4, 5)
     ewsrc (6, 7)
     subvl (8, 9)
     extra (10, 11, 12, 13, 14, 15, 16, 17, 18)
     mode (19, 20, 21, 22, 23)
     extra2 ((10, 11), (12, 13), (14, 15), (16, 17))
     smask (16, 17, 18)
     extra3 ((10, 11, 12), (13, 14, 15), (16, 17, 18))
Prefix.rm
     spr (6, 8, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31)
     mmode (6,)
     mask (8, 10, 11)
     elwidth (12, 13)
     ewsrc (14, 15)
     subvl (16, 17)
     extra (18, 19, 20, 21, 22, 23, 24, 25, 26)
     mode (27, 28, 29, 30, 31)
     extra2 ((18, 19), (20, 21), (22, 23), (24, 25))
     smask (24, 25, 26)
     extra3 ((18, 19, 20), (21, 22, 23), (24, 25, 26))

It takes an absolute eternity to make things done, really, not even hours, but days. Still, here's what we have for now (I've omitted the boilerplate):


class Instruction(_Fields):
    _: _Field = range(0, 32)
    major: _Field = range(0, 6)

class WordInstruction(Instruction):
    pass

class PrefixedInstruction(Instruction):
    class Prefix(WordInstruction):
        pass

    class Suffix(WordInstruction):
        pass

    prefix: Prefix = range(0, 32)
    suffix: Suffix = range(32, 64)
    major: _Field = Suffix.major

class SVP64Instruction(PrefixedInstruction):
    class Prefix(PrefixedInstruction.Prefix, _Prefix):
        pass

    prefix: Prefix


With the code above, SVP64 instructions from the binary file produce the representations like below:

SVP64Instruction(0x54000007c410214, major=0x1, prefix.insn=0x5400000, prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0x54000, prefix.rm.mmode=0x0, prefix.rm.mask=0x0, prefix.rm.elwidth=0x1, prefix.rm.ewsrc=0x1, prefix.rm.subvl=0x1, prefix.rm.extra=0x0, prefix.rm.mode=0x0, prefix.rm.extra2[0]=0x0, prefix.rm.extra2[1]=0x0, prefix.rm.extra2[2]=0x0, prefix.rm.extra2[3]=0x0, prefix.rm.smask=0x0, prefix.rm.extra3[0]=0x0, prefix.rm.extra3[1]=0x0, prefix.rm.extra3[2]=0x0, suffix.major=0x1)

Notice how we override fields positions (PrefixedIstruction.major: _Field = Suffix.major) and how we enforce more typing or similar actions (SVP64Instruction.prefix: Prefix). Both inheritance and field overriding act as one would expect. Took me two days to make it done, perhaps there are more updates ahead, but at least this seems like a good candidate for extensions.
Since it's possible to iterate over the fields, binutils will likely inherit these classes and do something like "for each field, generate C get/set functions for it; these are to be used in the non-generated part".

 290     pu: bool = False
 301         "PU": "pu",

do bear in mind these do not exist any more.
Pack/Unpack is now part of LDST normal crops modes

(In reply to Dmitry Selyutin from comment #3)

> With the code above, SVP64 instructions from the binary file produce the
> representations like below:
> 
> SVP64Instruction(0x54000007c410214, major=0x1, prefix.insn=0x5400000,
> prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0x54000,
> prefix.rm.mmode=0x0, prefix.rm.mask=0x0, prefix.rm.elwidth=0x1,

this is kinda slowly progressing towards full decode,
which will btw be needed for cavatools (to autogenerate
c data steucts and associated decoder, cavatools is a
c-based extremely fast ISA simulator).
although PowerDecoder2 is the canonical decoder as it
captures one hell of a lot of quirks.

(In reply to Dmitry Selyutin from comment #4)

> Since it's possible to iterate over the fields, binutils will likely inherit
> these classes and do something like "for each field, generate C get/set
> functions for it; these are to be used in the non-generated part".

Sounds perfect for cavatools. and PowerDecoderSubset.
although on PDS it is essential to fit with the existing
API which creates PowerOps(). PowerOp() critically relies
on column names from the CSV files to create its fields
(which are also dynamic)

There was an issue in the previous version, fixed now. Here's the assembly code I just checked:

sv.bc 2, 9, 8
sv.add 55, 66, 77
sv.add./mr *44, *35, *121

Considering the binary generated by binutils (either directly or indirectly via pysvp64asm), here's what we have with pysvp64dis applied:

.llong 0x540000040490008 # sv.bc
SVP64Instruction(0x540000040490008, major=0x1, prefix.insn=0x5400000, prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0x0, prefix.rm.mmode=0x0, prefix.rm.mask=0x0, prefix.rm.elwidth=0x0, prefix.rm.ewsrc=0x0, prefix.rm.subvl=0x0, prefix.rm.extra=0x0, prefix.rm.mode=0x0, prefix.rm.extra2[0]=0x0, prefix.rm.extra2[1]=0x0, prefix.rm.extra2[2]=0x0, prefix.rm.extra2[3]=0x0, prefix.rm.smask=0x0, prefix.rm.extra3[0]=0x0, prefix.rm.extra3[1]=0x0, prefix.rm.extra3[2]=0x0, suffix.major=0x10)

.llong 0x5400a407ee26a14 # sv.add
SVP64Instruction(0x5400a407ee26a14, major=0x1, prefix.insn=0x5400a40, prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0xa40, prefix.rm.mmode=0x0, prefix.rm.mask=0x0, prefix.rm.elwidth=0x0, prefix.rm.ewsrc=0x0, prefix.rm.subvl=0x0, prefix.rm.extra=0x52, prefix.rm.mode=0x0, prefix.rm.extra2[0]=0x0, prefix.rm.extra2[1]=0x2, prefix.rm.extra2[2]=0x2, prefix.rm.extra2[3]=0x1, prefix.rm.smask=0x2, prefix.rm.extra3[0]=0x1, prefix.rm.extra3[1]=0x2, prefix.rm.extra3[2]=0x2, suffix.major=0x1f)

.llong 0x54027a47d68f215 # sv.add
SVP64Instruction(0x54027a47d68f215, major=0x1, prefix.insn=0x54027a4, prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0x27a4, prefix.rm.mmode=0x0, prefix.rm.mask=0x0, prefix.rm.elwidth=0x0, prefix.rm.ewsrc=0x0, prefix.rm.subvl=0x0, prefix.rm.extra=0x13d, prefix.rm.mode=0x4, prefix.rm.extra2[0]=0x2, prefix.rm.extra2[1]=0x1, prefix.rm.extra2[2]=0x3, prefix.rm.extra2[3]=0x2, prefix.rm.smask=0x5, prefix.rm.extra3[0]=0x4, prefix.rm.extra3[1]=0x7, prefix.rm.extra3[2]=0x5, suffix.major=0x1f)

I also added a "special" __post_init__ method, like dataclasses do. The code we use now looks like this:

class PrefixedInstruction(Instruction):
    class Prefix(WordInstruction):
        class Error(ValueError):
            pass

        def __post_init__(self):
            if self.major != 0x1:
                raise self.__class__.Error(self.major)
            return super().__post_init__()

    class Suffix(WordInstruction):
        pass

    prefix: Prefix = range(0, 32)
    suffix: Suffix = range(32, 64)
    major: _Field = Suffix.major


class SVP64Instruction(PrefixedInstruction):
    class Prefix(PrefixedInstruction.Prefix, _Prefix):
        class Error(PrefixedInstruction.Prefix.Error):
            pass

        def __post_init__(self):
            if self.pid != 0b11:
                raise self.__class__.Error(self.pid)
            return super().__post_init__()

    prefix: Prefix


This can be even more reduced if I drop custom Error classes; however, I found it convenient to simply try to create SVP64Instruction and catch SVP64Instruction.Prefix.Error. If the error is caught, we resort to simple PrefixedInstruction (which, in turn, might raise its own exception, which can be also caught).

(In reply to Dmitry Selyutin from comment #8)
> There was an issue in the previous version, fixed now.
> .llong 0x540000040490008 # sv.bc
> SVP64Instruction(0x540000040490008, major=0x1, prefix.insn=0x5400000,
> prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0x0, prefix.rm.mmode=0x0,
> prefix.rm.mask=0x0, prefix.rm.elwidth=0x0, prefix.rm.ewsrc=0x0,
> prefix.rm.subvl=0x0, prefix.rm.extra=0x0, prefix.rm.mode=0x0,
> prefix.rm.extra2[0]=0x0, prefix.rm.extra2[1]=0x0, prefix.rm.extra2[2]=0x0,
> prefix.rm.extra2[3]=0x0, prefix.rm.smask=0x0, prefix.rm.extra3[0]=0x0,
> prefix.rm.extra3[1]=0x0, prefix.rm.extra3[2]=0x0, suffix.major=0x10)

Not entirely fixed yet; I'm still checking how to remap the major. I want a way to express that major is a Suffix.major, but obviously it treats Suffix as 32-bit (as it should).
The current solution is this:

    class Suffix(WordInstruction):
        pass

    prefix: Prefix = range(0, 32)
    suffix: Suffix = range(32, 64)
    major: _Field = Suffix.major.remap(suffix)

Looks somewhat verbose to me, but actually expresses what happens under the hood. The major field is a new field, made from same field of the Suffix class, but remapped with respect to how suffix is actually mapped.

So, the correct print is:

.llong 0x540000040490008 # sv.bc
SVP64Instruction(0x540000040490008, major=0x10, prefix.insn=0x5400000, prefix.major=0x1, prefix.pid=0x3, prefix.rm.spr=0x0, prefix.rm.mmode=0x0, prefix.rm.mask=0x0, prefix.rm.elwidth=0x0, prefix.rm.ewsrc=0x0, prefix.rm.subvl=0x0, prefix.rm.extra=0x0, prefix.rm.mode=0x0, prefix.rm.extra2[0]=0x0, prefix.rm.extra2[1]=0x0, prefix.rm.extra2[2]=0x0, prefix.rm.extra2[3]=0x0, prefix.rm.smask=0x0, prefix.rm.extra3[0]=0x0, prefix.rm.extra3[1]=0x0, prefix.rm.extra3[2]=0x0, suffix.major=0x10)

(In reply to Dmitry Selyutin from comment #10)

>     class Suffix(WordInstruction):
>         pass
> 
>     prefix: Prefix = range(0, 32)
>     suffix: Suffix = range(32, 64)
>     major: _Field = Suffix.major.remap(suffix)
> 
> Looks somewhat verbose to me, but actually expresses what happens under the
> hood. The major field is a new field, made from same field of the Suffix
> class, but remapped with respect to how suffix is actually mapped.

nice.

remember that you have to decode the suffix (partially), identifying
at the bare minimum the type of the operation (LDST/ALU/BRANCH/CROP)
before you can proceed to decoding either the EXTRA2/3 registers
or the Mode.

this means decoding MAJOR then XO.  no other information is needed
beyond that intermediary point for the purposes of getting the
Function (LDST/ALU/BRANCH/CROP)

Yet another day of banging my head towards metaclasses wall, but again, a success story!

1. We now have a really pretty print!
print(SVP64Instruction)
SVP64Instruction(major=(32, 33, 34, 35, 36, 37), prefix={(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31), insn=(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31), major=(0, 1, 2, 3, 4, 5), pid=(7, 9), rm={(6, 8, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31), spr=(6, 8, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31), mmode=(6,), mask=(8, 10, 11), elwidth=(12, 13), ewsrc=(14, 15), subvl=(16, 17), extra=(18, 19, 20, 21, 22, 23, 24, 25, 26), mode=(27, 28, 29, 30, 31), extra2[0]=(18, 19), extra2[1]=(20, 21), extra2[2]=(22, 23), extra2[3]=(24, 25), smask=(24, 25, 26), extra3[0]=(18, 19, 20), extra3[1]=(21, 22, 23), extra3[2]=(24, 25, 26)}}, suffix={(32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63), major=(32, 33, 34, 35, 36, 37)})

print(some_insn)
SVP64Instruction(0x54027a47d68f215, major=0x1f, prefix={0x54027a4, insn=0x54027a4, major=0x1, pid=0x3, rm={0x27a4, spr=0x27a4, mmode=0x0, mask=0x0, elwidth=0x0, ewsrc=0x0, subvl=0x0, extra=0x13d, mode=0x4, extra2[0]=0x2, extra2[1]=0x1, extra2[2]=0x3, extra2[3]=0x2, smask=0x5, extra3[0]=0x4, extra3[1]=0x7, extra3[2]=0x5}}, suffix={0x7d68f215, major=0x1f})


2. We now have a clear syntax to remap some field, as long as it is wrapped with a class.
class RM(Fields):
    """SVP64 RM: https://libre-soc.org/openpower/sv/svp64/"""
    _: Field = range(24)
    spr: Field = _
    mmode: Field = (0,)
    mask: Field = range(1, 4)
    elwidth: Field = range(4, 6)
    ewsrc: Field = range(6, 8)
    subvl: Field = range(8, 10)
    extra: Field = range(10, 19)
    mode: Field = range(19, 24)
    extra2: Field[4] = (
        range(10, 12),
        range(12, 14),
        range(14, 16),
        range(16, 18),
    )
    smask: Field = range(16, 19)
    extra3: Field[3] = (
        range(10, 13),
        range(13, 16),
        range(16, 19),
    )

class Prefix(Fields):
    """SVP64 Prefix: https://libre-soc.org/openpower/sv/svp64/"""
    _: Field = range(32)
    insn: Field = _
    major: Major.remap(range(0, 6))
    pid: PID.remap((7, 9))
    rm: RM.remap((6, 8) + tuple(range(10, 32)))


3. Any proper field intended to be inherited should declare a magic field which describes how the field is mapped in overall (exceptions: Fields base class, Instruction base class). This grows from a limitation, since otherwise class-level remap won't work; but, in fact, a nice bonus is that this mechanism can be used if someone tries to remap field with scheme containing less entries (e.g. if one remaps field of length 6 to new positions with 5 indices).

The next steps are:
1. Switch some parts of the code to new classes (deprecating the stuff I implemented inside selectable_int module).
2. Teach the classes to generate the masks: since fields positions are immutable, these masks can be used for getters/setters.
3. Inherit these classes in binutils and generate getters/setters.
4. Step-by-step extend the SVP64Instruction class with new fields and logic regarding how parts of the instruction are read.
5. Repeat the same logic in binutils, with the only difference that binutils will use the generated getters/setters.

(In reply to Dmitry Selyutin from comment #12)
> Yet another day of banging my head towards metaclasses wall, but again, a
> success story!
> 
> 1. We now have a really pretty print!

in which universe? :)
fascinatingly it looks like it's repr()able


> print(some_insn)
> SVP64Instruction(0x54027a47d68f215, major=0x1f, prefix={0x54027a4,
> insn=0x54027a4, major=0x1, pid=0x3, rm={0x27a4, spr=0x27a4, mmode=0x0,
> mask=0x0, elwidth=0x0, ewsrc=0x0, subvl=0x0, extra=0x13d, mode=0x4,

ahh, that's really handy.


> 2. We now have a clear syntax to remap some field, as long as it is wrapped
> with a class.

making some of these optional is probably not a good ides
except by an inheriting class.  or... maybe,i dunno.

> class RM(Fields):
>     elwidth: Field = range(4, 6)?

pack/unpack bits need to be added here.
pack bit 4
Unpack bit 5

> 3. Any proper field intended to be inherited should declare a magic field
> which describes how the field is mapped in overall 

can you give an example how that works? like an amoeba
this has grown to a multicellular organism :)

(In reply to Luke Kenneth Casson Leighton from comment #14)
> (In reply to Dmitry Selyutin from comment #12)
> making some of these optional is probably not a good ides
> except by an inheriting class.  or... maybe,i dunno.

No-no, these are not optional, don't think of dataclasses. If we have record like `x: Y = z`, it means "create field named x of type Y, remapped to z". More details below.

> pack/unpack bits need to be added here.
> pack bit 4
> Unpack bit 5

I'll add these, thanks for reminder!

> can you give an example how that works? like an amoeba
> this has grown to a multicellular organism :)

Sure! The Fields class basically subclasses FieldSelectableInt, but also checks for some -- unsurprisingly -- class-level immutable fields. Here are Major, PID, RM and Prefix classes.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_fields.py;h=98d2a5ae4e8800326fcf3c3d64d15e4eecd8be61;hb=cdd9b4ace0d8417c4caa49741046bf75dfb6b112#l288

Major and PID don't have subfields, so we simply declare these with the only field (the magic field I mentioned). PID (2 bits) perhaps doesn't need a class and can be class-less field; as for Major (6 bits), it's used in another place (Instruction and PrefixedInstruction classes), and it's remapped differently in these. We also declare RM field (24 bits), with some additional fields.

Now, let's take a look at the Prefix class. We know it contains "rm" RM field, but the bits are mapped non-sequentially, unlike in RM vanilla class. What do we do? We declare the RM field as remapped RM:
https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_fields.py;h=98d2a5ae4e8800326fcf3c3d64d15e4eecd8be61;hb=cdd9b4ace0d8417c4caa49741046bf75dfb6b112#l327

This basically says "look at the RM class field, iterate over all its subfields, and make so that these subfields are all remapped appropriately". So, for example, the RM.mask field in the new class is remapped from (1, 2, 3) to (8, 10, 11). And so for other fields. An alternative way to remap would be, instead of `rm: RM.remap(...)`, to do `rm: Field = ...`. This is just a syntactic sugar.

Now, let's take a look at how we handle instructions...

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_insn.py;h=03a134d041d0826567640006696c7e61b4d43cb8;hb=b187c5c0c6c9b728f450bd0c129018a42be9637a#l539

Here we have Instruction class. It inherits from Fields, but it's flexible (size of 0 bits). Nobody should instantiate this class directly. Still, it does provides some instruction-agnostic stuff (e.g. way to obtain the corresponding database entry).

Next, we have WordInstruction class.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_insn.py;h=03a134d041d0826567640006696c7e61b4d43cb8;hb=b187c5c0c6c9b728f450bd0c129018a42be9637a#l570

It already has defined size of 32 bits, and also provides the "major" field. The major is mapped exactly from 0 to 6 bits, like the parent class. I inherited it one more time in case I need to extend the Major at power_insn level (and also for a reason below). Here we also have `disassemble` method (which by the way accesses instruction-type-agnostic dbrecord).

Now, we want to have a prefixed instruction, which we represent for now as a pair of WordInstructions.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_insn.py;h=03a134d041d0826567640006696c7e61b4d43cb8;hb=b187c5c0c6c9b728f450bd0c129018a42be9637a#l589

Unlike the usual Prefix class from field, only suitable for interpreting, we also want to ensure that the major field is OK. So we define our Prefix class and in its __post_init__ check that the major field equals to 0x1. We map this prefix at bits 0..31.

Then, we have a suffix, also with a custom class (for no real purpose yet, only for explicitness). We map the suffix at bits 32..63.

Next, we provide our own major field, mapped to the same place PrefixedInstruction.suffix field is. We go to PrefixedInstruction.Suffix (inherited from the WordInstruction), then access Major (obtained from WordInstruction.Major via inheritance), and then map this Major to the place we need (that is, PrefixedInstruction not only has prefix.major and suffix.major, but also a major alias referring to suffix.major).

We also tune disassemble method.

Now, to gory SVP64 details!

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_insn.py;h=03a134d041d0826567640006696c7e61b4d43cb8;hb=b187c5c0c6c9b728f450bd0c129018a42be9637a#l622

Actually not that gory. We inherit PrefixedInstruction class, so we have everything it has. We only tune the Prefix class and prefix field so that they check PID field. And we also tune disassemble method (notice this "sv").

This all is used in pysvp64asm.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/sv/trans/pysvp64dis.py;h=23e02859d88a9237da3b0ac899897c2915ec1994;hb=refs/heads/pysvp64dis#l33

We read the WordInstruction (potentially a prefix). If the WordInstruction has non-prefix major, we return this instruction. Otherwise we try reading the suffix and instantiate either SVP64 or other prefixed instruction.

Ah, yeah, a small detail useful for understanding of what say RM.remap does: it creates a new class inherited from RM but with fields mapped in a different way. This is it.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/power_fields.py;h=98d2a5ae4e8800326fcf3c3d64d15e4eecd8be61;hb=b187c5c0c6c9b728f450bd0c129018a42be9637a#l326

pid should not really be called pid, it is unique to
svp64.  svp64id would be more appropriate

Since it's SVP64-specific, as well as RM, the Prefix class in power_fields.py is SVP64-specific. I will refactor these bits then, reorganizing the hierarchy.

Yet another iteration, summary below.

1. From now on, we indeed pass the reference to the same selectable int throughout each and every field.
2. All fields are immutable; there are no real instances that need to be created upon field access, except for the thin references to the field.
3. Fixed many issues, including field assignment, field iteration, boundary checks, representation, etc.
4. Massive code cleanup; now this should be quite straightforward to follow it (assuming the reader can dig up the concepts like metaclasses and descriptors).
5. Migrated parts of the code: power_insn, pysvp64asm.

test_caller_svp64.py is OK.

Definitions for the new classes:
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=c2f5bc883ca4f12ccf7f9145c2ccd9be3d989bc3

Expected usage for the new classes:
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=0753414eea9bbd9bd6380969b4356d907c1dcbc4

Again, note that it is no longer needed to specify the size of the integer manually. The classes take care of these checks and possible conversions.

looks really clean. v. late. more tomorrow. bruef:

412 class Record:
 413     name: str
 414     rc: bool

needs renaming to avoid nmigen nameclash.

What's the issue with naming? Anyone can import this as `power_insn.Record`, or as `from power_insn import Record as Cocojumbo`.
Also note that Record does not relate to new field classes, it's just an instruction database description.

(In reply to Dmitry Selyutin from comment #21)
> What's the issue with naming? Anyone can import this as `power_insn.Record`,
> or as `from power_insn import Record as Cocojumbo`.

yes.  this is extra effort and if not done results in confusion.

also i spotted "id" which is a keyword in python so should also
be avoided.  (vim auto-highlights keywords as special-cases)

(In reply to Luke Kenneth Casson Leighton from comment #23)
> (In reply to Dmitry Selyutin from comment #21)
> > What's the issue with naming? Anyone can import this as `power_insn.Record`,
> > or as `from power_insn import Record as Cocojumbo`.
> 
> yes.  this is extra effort and if not done results in confusion.

IIRC Python's PEPs recommend that nobody uses "star" imports. This is a hard "no-no".

> also i spotted "id" which is a keyword in python so should also
> be avoided.  (vim auto-highlights keywords as special-cases)

Yeah I renamed "pid" since you mentioned it. Since it already resided at svp64 space, I chose id instead of svp64id.

That said, I'm OK with renaming the Record, looking for an alternative naming. Instances of Record are never to be instantiated directly, though, only via the database.

(In reply to Dmitry Selyutin from comment #25)
> That said, I'm OK with renaming the Record, looking for an alternative
> naming. Instances of Record are never to be instantiated directly, though,
> only via the database.

ah pffh then don't worry about it, at all, if it's internal.

(In reply to Dmitry Selyutin from comment #24)

> IIRC Python's PEPs recommend that nobody uses "star" imports. This is a hard
> "no-no".

uh-huhn.  at some point i am going to have to do a massive (probably automated)
wildcard-replace on nmigen. not looking forward to it.

Updates:

* Marked all pseudo-fields as Padding. This is needed to make the code work, since the underlying logic checks the size of the mapping.

* Now we can traverse the whole mapping, including all nested fields. This code will be used to generate getters/setters for all fields.
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=ddeb5f3ebb1beaf9b1fd3414ad69829f39d0c85f

* Switched caller.py to new fields.
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=bc14972f21dca9e7008b3be413d6891d4c45752a

* Removed the legacy "mapping" stuff.
https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=2fc03dde65cb050921e0564b9579caac9b4867c2

* Fixed some subtle issues and refactored some code.

Here's what we have at this stage. The output is obviously huge, so I'll limit it to an illustrative example.

struct svp64_insn {
    uint64_t value;
};

static inline uint64_t
svp64_insn_get_prefix_id(const struct svp64_insn *insn)
{
  uint64_t value = insn->value;

  return (
    (((value >> UINT64_C(56)) & UINT64_C(1)) << UINT64_C(0)) |
    (((value >> UINT64_C(54)) & UINT64_C(1)) << UINT64_C(1)) |
    UINT64_C(0)
  );
}

static inline void
svp64_insn_set_prefix_id(struct svp64_insn *insn, uint64_t value)
{
  insn->value = (
    (((value >> UINT64_C(0)) & UINT64_C(1)) << UINT64_C(56)) |
    (((value >> UINT64_C(1)) & UINT64_C(1)) << UINT64_C(54)) |
    UINT64_C(0)
  );
}

I will have to refactor the code we had previously for remapping extras, but, other than that, I think we can return to the disassembly support.

After a long and boring per-day debug, I've finally made binutils deal with the new fields.
Here's how it eventually looks:

static void
svp64_assemble (char *str)
{
  struct svp64_ctx svp64;

  if (setjmp (svp64_exception) != 0)
    return;

  memset (&svp64, 0, sizeof (svp64));

  svp64_decode (str, &svp64);
  svp64_validate_and_fix (&svp64);

  svp64_insn_set_prefix_PO (&svp64.insn, 0x1);
  svp64_insn_set_prefix_id (&svp64.insn, 0x3);

  svp64_insn_set_prefix_rm_mode (&svp64.insn, svp64.mode);
  if (svp64.desc->sv_ptype == SVP64_PTYPE_P2)
    svp64_insn_set_prefix_rm_smask (&svp64.insn, svp64.smask);
  svp64_insn_set_prefix_rm_mmode (&svp64.insn, svp64.mmode);
  svp64_insn_set_prefix_rm_mask (&svp64.insn, svp64.pmask);
  svp64_insn_set_prefix_rm_subvl (&svp64.insn, svp64.subvl);
  svp64_insn_set_prefix_rm_ewsrc (&svp64.insn, svp64.srcwid);
  svp64_insn_set_prefix_rm_elwidth (&svp64.insn, svp64.destwid);

  ppc_assemble (str, &svp64);
}

Once we collected the SVP64 context (and also partially initialized it), we "jump" to common code in ppc_assemble, which takes care of operands. Whenever there's an operand, we "remap" it (I use "remap" wording even when the operand stands unchanged). The remapping process also affects the `svp64.insn` field; the logic there is obviously longer than the code above, since we also have to take care of EXTRA2/EXTRA3 schemes.

For the reference, here's how the disassembly looks like:

static const struct powerpc_opcode *
svp64_lookup (uint64_t insn, ppc_cpu_t dialect,
    struct svp64_ctx *svp64)
{
  uint32_t suffix;
  unsigned long op;
  const struct powerpc_opcode *opcode;
  const struct svp64_record *record;
  const struct svp64_record *record_end;

  svp64_insn_set (&svp64->insn, insn);

  if ((svp64_insn_get_prefix_PO (&svp64->insn) != 0x1) ||
      (svp64_insn_get_prefix_id (&svp64->insn) != 0x3))
    return NULL;

  suffix = (uint32_t)svp64_insn_get_suffix (&svp64->insn);
  opcode = lookup_powerpc (suffix, dialect & ~PPC_OPCODE_ANY);
  if (opcode == NULL && (dialect & PPC_OPCODE_ANY) != 0)
    opcode = lookup_powerpc (suffix, dialect);
  if (opcode == NULL)
    return NULL;

  op = SVP64_OP (suffix);
  record_end = (svp64_records + svp64_record_indices[op + 1]);
  for (record = svp64_records + svp64_record_indices[op];
       record < record_end;
       ++record)
    {
      /* binutils consider Rc bit to be a part of the mask.
	 SVP64, however, has per-instruction mask.  */
      if ((record->opcode.value & record->opcode.mask) ==
	    ((opcode->opcode & opcode->mask) & record->opcode.mask))
	break;
    }
  if (record == record_end)
    return NULL;

  svp64->desc = &record->desc;

  return opcode;
}

The next step is to start filling other bits of SVP64 context, not only `desc` and `insn`. Since recent we already have `function` field, so this should give us a good start.

These instructions appear only in markdown files:
['addex', 'addpcis', 'ba', 'bca', 'bcctrl', 'bcl', 'bcla', 'bclrl', 'bctarl', 'bl', 'bla', 'divmod2du', 'ffadd', 'ffadd.', 'ffdiv', 'ffdiv.', 'ffdivs', 'ffdivs.', 'ffmul', 'ffmul.', 'ffmuls', 'ffmuls.', 'ffsub', 'ffsub.', 'ffsubs', 'ffsubs.', 'lfiwix', 'lmw', 'lq', 'lswi', 'lswx', 'madded', 'maddhd', 'maddhdu', 'maddld', 'rfscv', 'scv', 'stmw', 'stq', 'stswi', 'stswx', 'sv.bc', 'sv.bca', 'sv.bcl', 'sv.bcla', 'sv.bclr', 'sv.bclrl']

These instructions do not appear in SVP64 CSV files ("^(?:LDST)?RM-(1P|2P)-.*?\.csv$"):
['b', 'bcctr', 'bctar', 'darn', 'dcbz', 'fabs', 'fabs.', 'fcpsgn', 'fcpsgn.', 'fmr', 'fmr.', 'fnabs', 'fnabs.', 'fneg', 'fneg.', 'hrfid', 'mcrxrx', 'mfcr', 'mfmsr', 'mfocrf', 'mtcrf', 'mtmsr', 'mtmsrd', 'mtocrf', 'rfid', 'sc', 'setvl', 'setvl.', 'svindex', 'svremap', 'svshape', 'td', 'tlbie', 'tw']

(In reply to Dmitry Selyutin from comment #31)

> These instructions do not appear in SVP64 CSV files
> ("^(?:LDST)?RM-(1P|2P)-.*?\.csv$"):
> ['b', 'bcctr', 'bctar', 'darn', 'dcbz', 'fabs', 'fabs.', 'fcpsgn',
> 'fcpsgn.', 'fmr', 'fmr.', 'fnabs', 'fnabs.', 'fneg', 'fneg.', 'hrfid',
> 'mcrxrx', 'mfcr', 'mfmsr', 'mfocrf', 'mtcrf', 'mtmsr', 'mtmsrd', 'mtocrf',
> 'rfid', 'sc', 'setvl', 'setvl.', 'svindex', 'svremap', 'svshape', 'td',
> 'tlbie', 'tw']

yes none of these are a surprise, the setvl svindex svremap svshape ones
are "management" (for which 64-bit variants haven't been defined).

mtmsr you absolutely cannot move more than one MSR (Machine Status Register)
so there is absolutely no point vectorising it.

rfid and sc are "return from interrupt" and "system call" respectively,
it is insanity to consider vectorising them.

branch (b) unconditionally is impossible to vectorise, how can you "branch"
to multiple locations?

others just haven't yet had an analysis done on them (fmr etc.)

(In reply to Luke Kenneth Casson Leighton from comment #32)
> (In reply to Dmitry Selyutin from comment #31)
> 
> > These instructions do not appear in SVP64 CSV files
> yes none of these are a surprise

What about those present only in markdown?

OK, I have a real breakthrough. It seems we can now construct fields better, relying on the parsed operands. More details: https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=adefa3af2343f5bce4bd8048db4a54f4a2bf355c.

Below are static operands and opcodes for all instructions which start with "add". I think, with this code, the only thing left for vanilla PPC assembler is to decode the dynamic operands.

addmeo.
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c0005d5, mask=0xfc0007ff)
add.
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000215, mask=0xfc0007ff)
addi
    FieldsOpcode(value=0x38000000, mask=0xfc000000)
addis
    FieldsOpcode(value=0x3c000000, mask=0xfc000000)
addc.
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000015, mask=0xfc0007ff)
addme.
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c0001d5, mask=0xfc0007ff)
addeo
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000514, mask=0xfc0007ff)
addco
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000414, mask=0xfc0007ff)
addzeo
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000594, mask=0xfc0007ff)
add
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000214, mask=0xfc0007ff)
addeo.
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000515, mask=0xfc0007ff)
addme
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c0001d4, mask=0xfc0007ff)
addo
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000614, mask=0xfc0007ff)
addco.
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000415, mask=0xfc0007ff)
addmeo
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c0005d4, mask=0xfc0007ff)
adde
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000114, mask=0xfc0007ff)
addo.
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000615, mask=0xfc0007ff)
addzeo.
    StaticOperand(name='OE', value=1)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000595, mask=0xfc0007ff)
addic
    FieldsOpcode(value=0x30000000, mask=0xfc000000)
adde.
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000115, mask=0xfc0007ff)
addic.
    FieldsOpcode(value=0x34000000, mask=0xfc000000)
addze
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000194, mask=0xfc0007ff)
addze.
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=1)
    FieldsOpcode(value=0x7c000195, mask=0xfc0007ff)
addc
    StaticOperand(name='OE', value=0)
    StaticOperand(name='Rc', value=0)
    FieldsOpcode(value=0x7c000014, mask=0xfc0007ff)
addg6s
    FieldsOpcode(value=0x7c000094, mask=0xfc0007fe)

(In reply to Dmitry Selyutin from comment #33)
> (In reply to Luke Kenneth Casson Leighton from comment #32)
> > (In reply to Dmitry Selyutin from comment #31)
> > 
> > > These instructions do not appear in SVP64 CSV files
> > yes none of these are a surprise
> 
> What about those present only in markdown?

addex simply appears to be... missing. no idea why
https://github.com/antonblanchard/microwatt/blob/26095986f3ff38eed950dbbae306c13c86815b6e/decode1.vhdl#L73

lkcl@fizzy:~/src/libresoc/openpower-isa$ grep bcctr openpower/*/*.mdwn
openpower/isa/branch.mdwn:* bcctr BO,BI,BH (LK=0)
openpower/isa/branch.mdwn:* bcctrl BO,BI,BH (LK=1)

bcctrl is covered by bcctr.  LK is decoded entirely separately.

ignore it for now as the sv.{branches} needs to be looked at
properly as a whole block.

https://libre-soc.org/irclog/%23libre-soc.2022-08-30.log.html#t2022-08-30T20:27:26

better reply about LK=1

Surprisingly I got the first encouraging results for "word" instructions.
This is what we had with binutils...
0000000000000000 <.text>:
   0:   14 6a e2 7e     add     r23,r2,r13
   4:   15 f2 68 7d     add.    r11,r8,r30

...and here's what we have with pysvp64dis:
add 23,2,13
add. 11,8,30

There's for sure much to be done yet, I'm pretty sure, but still, I'm surprised.

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=5808ac2c19882ec2977d6608b5cead2e741c3e97

(In reply to Dmitry Selyutin from comment #37)

> ...and here's what we have with pysvp64dis:
> add 23,2,13
> add. 11,8,30

apart from "r", perfect!

irony to have to do a full 32bit decode just because
of the register naming in svp64. i was expecting only
to be able to part-decode then hand over to mainline
binutils.

(In reply to Luke Kenneth Casson Leighton from comment #38)
> (In reply to Dmitry Selyutin from comment #37)
> 
> > ...and here's what we have with pysvp64dis:
> > add 23,2,13
> > add. 11,8,30
> 
> apart from "r", perfect!

I don't see this "r" in any of our sources. Moreover, even binutils have a switch to stop printing it.

> irony to have to do a full 32bit decode just because
> of the register naming in svp64. i was expecting only
> to be able to part-decode then hand over to mainline
> binutils.

Actually for binutils things are simpler: they have most parts of what I had to create here. For example, they already have a comfortable way to find the instruction operands. And, unlike with our code, this is grouped in one place -- libopcode.

Since recent commits we have this code for disassebly:

b 0x28
add r23,r2,r13
add. r11,r8,r30

This is made possible due to custom operands classes.

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

https://git.libre-soc.org/?p=openpower-isa.git;a=commit;h=158a65aa8975c7d6a2b3107ae1d1ad834504e598

(In reply to Dmitry Selyutin from comment #40)
> Since recent commits we have this code for disassebly:
> 
> b 0x28

i just updated the markdown, the branch disasm should
not really attempt to resolve the address locally,
(unlike in binutils which calculates it from PC i believe?)
target_addr now no longer exists and i have notified the
OPF ISA WG by raising an issue.

https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/caller.py;hb=HEAD#l1062

btw the above is the "workaround" explaining comment #35.

1062         if hasattr(self.dec2.e.do, "lk"):
1063             lk = yield self.dec2.e.do.lk
1064             if lk:
1065                 asmop += "l"
1066         log("int_op", int_op)
1067         if int_op in [MicrOp.OP_B.value, MicrOp.OP_BC.value]:
1068             AA = yield self.dec2.dec.fields.FormI.AA[0:-1]
1069             log("AA", AA)
1070             if AA:
1071                 asmop += "a"

and just above it for Rc=1 and also OE.

"read and weep" i think is the most appropriate reaction.
the "correct" thing to do here would be to add CSV file
entries for all the missing assembly ops: ba, bla, etc
but if you can tolerate the above then i think it would
be less disruptive than adding new CSV files right now