RE: [PATCH v3 25/30] target/ppc: Move ADDI, ADDIS to decodetree, implement PADDI

[Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>]

From: Richard Henderson <richard.henderson@linaro.org>
> On 4/30/21 4:23 AM, Luis Fernando Fujita Pires wrote:
> > I think we should reconsider using the same .decode file for both 32-
> > and 64-bit instructions, to avoid duplicating argument set
> > definitions, and to keep the prefixed instructions close to their non-prefixed
> counterparts.
> varinsnwidth assumes there is no easy way to determine, before decoding, the
> width of the instruction.  The way this is implemented in decodetree is vastly less
> optimal than what we can do with a few lines for ppc.

I tried to solve this with one of the previous decodetree patches ("decodetree: Allow custom var width load functions"), whose goal was to allow us to implement a custom instruction load function (in reality, the only effect it had inside decodetree.py was to not generate the _load function).
So the instruction load would still be handled by a simple function inside translate.c, but we would use the auto-generated decode() function to call the trans_XX() functions.

> In addition, there's a rough spot in %field definitions.  You can't share those
> between patterns of different sizes, which can get confusing.  Have a look at
> target/rx, and the definitions of %b[23]_r_0, which is the same field for 2 and 3-
> byte insns.

Right. In the current patch we're already using separate definitions for 'si' depending on the format (%pls_si and %ds_si below):

&PLS_D          rt ra si:int64_t r:bool

%pls_si         32:s18 0:16
@PLS_D          ...... .. ... r:1 .. .................. \
                ...... rt:5 ra:5 ................       \
                &PLS_D si=%pls_si

@PLS_D_32       ...... rt:5 ra:5 si:s16                 &PLS_D r=0

%ds_si          2:s14  !function=times_4
@PLS_DS_32      ...... rt:5 ra:5 .............. ..      &PLS_D si=%ds_si r=0

And I also had to create separate @formats for 32- and 64-bit versions (@PLS_D, @PLS_D_32, etc.), which isn't that nice either.

> The replication of argument set definitions is unfortunate, but in the end will
> only be a handful of lines.  We could probably come up with a way to avoid that
> too, via a decodetree extension, if you really insist.  (My vague idea there would
> put the argument set definitions into a 3rd file, included on the decodetree
> command-line.)

I think we can already pass multiple files to decodetree.py and it will handle them correctly. I just didn't find a way to do that from the meson build files, which assume decodetree will always use a single input file.
Another option would be to allow files to be included from inside other .decode files.

> > And, in order to share the trans_PADDI/ADDI implementation, maybe add
> something to decodetree.py to allow us to specify that an instruction shares the
> trans_XX() implementation from another one, such as:
> > ADDI            001110 ..... ..... ................     @PLS_D_32 !impl=PADDI
> 
> This is done by using the same name up front.
> If you like, add a comment to give the real instruction name.
> 
> PADDI   001110 ..... ..... ................     @PLS_D_32 # ADDI
> 
> 
> > This way, we could (and would need to, in fact) keep the 'P' in the prefixed
> instruction names, but at the same time avoid having extra trans_XX functions
> just calling another one without any additional code.
> 
> I don't understand this at all.

Not a big deal. I was just referring to the fact that, in the current patch, you noted that the instruction names in insn64.decode were not prefixed by "P" due to the code sharing with the 32-bit instructions.

Thanks!
Luis