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

[Claudio Fontana]

On 16.09.2013 17:32, Richard Henderson wrote:
> 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 <address@hidden>
>>> ---
>>>  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.

Ok.

> 
>> 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.

I know, I did not express it well. What I meant is that it is impossible to 
misunderstand what is supposed to be passed to the function.
Not that it is impossible to willingly do so. 

> 
>> 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).

No need to do force explicit typechecking like that.
That kind of use of the preprocessor really hurts.
The only thing that needs to be addressed is to ensure that the programmer 
calling the function can quickly know for sure which instructions are ok to 
pass and which not.

Maybe we can categorize the instructions by order of appearance in the enum, 
coupled with an appropriate prefix.
For example INSN_RDNM_ADD, INSN_RDNM_SUB, ...

> 
> 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?

No, not yet.

> 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.

I agree, lets just keep only left shift and right shift.
Distinguishing the two costs one comparison per call, I think we can survive it.

C.