Bug 898 - binutils svp64 objdump support
Summary: binutils svp64 objdump support
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: High enhancement
Assignee: Dmitry Selyutin
URL: https://git.libre-soc.org/?p=binutils...
Depends on:
Blocks:
 
Reported: 2022-07-31 16:46 BST by Luke Kenneth Casson Leighton
Modified: 2022-09-24 20:17 BST (History)
1 user (show)

See Also:
NLnet milestone: NLNet.2019.10.042.Vulkan
total budget (EUR) for completion of task and all subtasks: 2500
budget (EUR) for this task, excluding subtasks' budget: 2500
parent task for budget allocation: 252
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
ghostmansd = {amount=2500, submitted=2022-09-16, paid = 2022-09-23}


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Kenneth Casson Leighton 2022-07-31 16:46:32 BST
now that binutils is being used it needs disasm/objdump support.
Comment 1 Dmitry Selyutin 2022-08-19 16:07:05 BST
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.
Comment 2 Dmitry Selyutin 2022-08-19 16:08:01 BST
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))
Comment 3 Dmitry Selyutin 2022-08-20 23:03:11 BST
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)
Comment 4 Dmitry Selyutin 2022-08-20 23:08:57 BST
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".
Comment 5 Luke Kenneth Casson Leighton 2022-08-20 23:25:05 BST
 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
Comment 6 Luke Kenneth Casson Leighton 2022-08-20 23:30:38 BST
(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.
Comment 7 Luke Kenneth Casson Leighton 2022-08-20 23:34:46 BST
(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)
Comment 8 Dmitry Selyutin 2022-08-21 10:52:53 BST
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)
Comment 9 Dmitry Selyutin 2022-08-21 10:57:27 BST
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).
Comment 10 Dmitry Selyutin 2022-08-21 11:08:10 BST
(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)
Comment 11 Luke Kenneth Casson Leighton 2022-08-21 11:37:22 BST
(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)
Comment 12 Dmitry Selyutin 2022-08-21 19:31:39 BST
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).
Comment 13 Dmitry Selyutin 2022-08-21 19:38:34 BST
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.
Comment 14 Luke Kenneth Casson Leighton 2022-08-21 20:47:22 BST
(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 :)
Comment 15 Dmitry Selyutin 2022-08-21 21:32:12 BST
(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.
Comment 16 Dmitry Selyutin 2022-08-21 21:38:07 BST
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.
Comment 17 Luke Kenneth Casson Leighton 2022-08-22 00:04:09 BST
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
Comment 18 Dmitry Selyutin 2022-08-22 06:20:16 BST
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.
Comment 19 Dmitry Selyutin 2022-08-26 23:40:00 BST
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.
Comment 20 Luke Kenneth Casson Leighton 2022-08-26 23:55:08 BST
looks really clean. v. late. more tomorrow. bruef:

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

needs renaming to avoid nmigen nameclash.
Comment 21 Dmitry Selyutin 2022-08-27 07:15:00 BST
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.
Comment 22 Dmitry Selyutin 2022-08-27 07:15:21 BST

    
Comment 23 Luke Kenneth Casson Leighton 2022-08-27 10:39:56 BST
(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)
Comment 24 Dmitry Selyutin 2022-08-27 18:49:19 BST
(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.
Comment 25 Dmitry Selyutin 2022-08-27 18:52:38 BST
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.
Comment 26 Luke Kenneth Casson Leighton 2022-08-27 19:03:46 BST
(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.
Comment 27 Dmitry Selyutin 2022-08-28 14:17:47 BST
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.
Comment 28 Dmitry Selyutin 2022-08-29 00:28:55 BST
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)
  );
}
Comment 29 Dmitry Selyutin 2022-08-29 00:30:16 BST
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.
Comment 30 Dmitry Selyutin 2022-08-29 17:01:58 BST
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.
Comment 31 Dmitry Selyutin 2022-08-30 17:06:54 BST
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']
Comment 32 Luke Kenneth Casson Leighton 2022-08-30 17:35:46 BST
(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.)
Comment 33 Dmitry Selyutin 2022-08-30 20:04:52 BST
(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?
Comment 34 Dmitry Selyutin 2022-08-30 20:11:06 BST
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)
Comment 35 Luke Kenneth Casson Leighton 2022-08-30 20:26:03 BST
(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.
Comment 36 Luke Kenneth Casson Leighton 2022-08-30 20:30:32 BST
https://libre-soc.org/irclog/%23libre-soc.2022-08-30.log.html#t2022-08-30T20:27:26

better reply about LK=1
Comment 37 Dmitry Selyutin 2022-08-30 20:37:46 BST
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
Comment 38 Luke Kenneth Casson Leighton 2022-08-31 01:08:55 BST
(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.
Comment 39 Dmitry Selyutin 2022-08-31 08:25:15 BST
(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.
Comment 40 Dmitry Selyutin 2022-08-31 11:49:41 BST
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
Comment 41 Luke Kenneth Casson Leighton 2022-08-31 15:37:37 BST
(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.
Comment 42 Luke Kenneth Casson Leighton 2022-08-31 15:51:44 BST
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