Re: [Qemu-devel] [PATCH v4 08/33] tcg-aarch64: Introduce tcg_fmt_Rdnm and tcg_fmt_Rdnm_lsl

[Richard Henderson <rth@twiddle.net>]

On 09/16/2013 01:41 AM, Claudio Fontana wrote:
> On 14.09.2013 23:54, Richard Henderson wrote:
>> Now that we've converted opcode fields to pre-shifted insns, we
>> can merge the implementation of arithmetic and shift insns.
>>
>> Simplify the left/right shift parameter to just the left shift
>> needed by tcg_out_tlb_read.
>>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>  tcg/aarch64/tcg-target.c | 78 +++++++++++++++++++++++-------------------------
>>  1 file changed, 38 insertions(+), 40 deletions(-)
>>
>> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c
>> index cc56fe5..0e7b67b 100644
>> --- a/tcg/aarch64/tcg-target.c
>> +++ b/tcg/aarch64/tcg-target.c
>> @@ -302,6 +302,30 @@ static inline uint32_t tcg_in32(TCGContext *s)
>>      return v;
>>  }
>>  
>> +/*
>> + * Encode various formats.
>> + */
>> +
>> +/* This function can be used for both Arithmetic and Logical (shifted register)
>> +   type insns.  Since we don't actually use the other available shifts, we only
>> +   support LSL here.  */
>> +static inline void tcg_fmt_Rdnm_lsl(TCGContext *s, AArch64Insn insn,
>> +                                    TCGType sf, TCGReg rd, TCGReg rn,
>> +                                    TCGReg rm, int imm6)
>> +{
>> +    /* Note that LSL is bits {23,22} = 0.  */
>> +    tcg_out32(s, insn | sf << 31 | imm6 << 10 | rm << 16 | rn << 5 | rd);
>> +}
>> +
>> +/* This function can be used for most insns with 2 input registers and one
>> +   output register.  This includes Arithmetic (shifted register, sans shift),
>> +   Logical, Shift, Multiply, Divide, and Bit operation.  */
>> +static inline void tcg_fmt_Rdnm(TCGContext *s, AArch64Insn insn, TCGType sf,
>> +                                TCGReg rd, TCGReg rn, TCGReg rm)
>> +{
>> +    tcg_out32(s, insn | sf << 31 | rm << 16 | rn << 5 | rd);
>> +}
>> +
> 
> The name of the function should reflect the fact that we are actually emitting instructions,
> not only formatting them. Also I despise mixed case in functions.
> So theoretically, tcg_out_rdnm.

Ok.

> I guess the question would be, are all instructions formatted exactly that
> way arithmetic and logical shifted register instructions of some sort?

The functino comment lists the insn groups from the manual to which the
function may be applied: Arithemetic, Logical, Shift, Multiply, Divide, Bit
operation.

> If so, I'd go with tcg_out_arith or similar. If not, we can say tcg_out_rdnm.

rdmn it is then.

> The previous implementation made it impossible to pass wrong opcodes to the
> function, since the opcode for the arith was a separate type.

No, this isn't C++.  Enumeration checks like that don't happen for C.

> It made it obvious to the reader in which cases the function can be used.
> We would lose this with this change here (combined with the INSN change).

Perhaps, perhaps not.

It would have been handy if ARM had officially assigned identifiers to the
formats, like Power, S390, and ia64 do.  Then one can build in the format ids
into both the function and enumeration names and use the preprocessor for
typechecking (c.f. the tcg_out_insn macro in  tcg/s390/tcg-target.c).

But without those format ids being official, inventing a set of format names
may be more confusing than not.  I'm not sure.

> Also we lose the ability to do right-shifted arithmetic operations, which I
>feel we should provide for completeness and to reduce the pain for the
>programmer who will eventually need them.

Nor do we provide ASR or ROR shifts; should we provide those too?  Please think
about what situations in which those would be useful.  Also think about the one
operation at a time nature of TCG.

My guess is that, beyond the one explicit use in the tlb, we could only make
use of shifted operations if TCG grew some sort of peephole optimizer so that
we can look across single operations.  And I don't ever see that happening.

Therefore I think adding LSR, ASR and ROR shifts is both a waste of time and
bloats the backend.


r~