Re: [Qemu-devel] [PATCH v4 11/33] tcg-aarch64: Handle constant operands to add, sub, and compare

[Claudio Fontana]

On 16.09.2013 17:45, Richard Henderson wrote:
> On 09/16/2013 02:02 AM, Claudio Fontana wrote:
>>> -static inline void tcg_out_cmp(TCGContext *s, TCGType ext, TCGReg rn,
>>> -                               TCGReg rm)
>>> +static void tcg_out_cmp(TCGContext *s, TCGType ext, TCGReg a,
>>> +                        tcg_target_long b, bool const_b)
>>>  {
>>> -    /* Using CMP alias SUBS wzr, Wn, Wm */
>>> -    tcg_fmt_Rdnm(s, INSN_SUBS, ext, TCG_REG_XZR, rn, rm);
>>> +    if (const_b) {
>>> +        /* Using CMP or CMN aliases.  */
>>> +        AArch64Insn insn = INSN_SUBSI;
>>> +        if (b < 0) {
>>> +            insn = INSN_ADDSI;
>>> +            b = -b;
>>> +        }
>>> +        tcg_fmt_Rdn_aimm(s, insn, ext, TCG_REG_XZR, a, b);
>>> +    } else {
>>> +        /* Using CMP alias SUBS wzr, Wn, Wm */
>>> +        tcg_fmt_Rdnm(s, INSN_SUBS, ext, TCG_REG_XZR, a, b);
>>> +    }
>>>  }
>>
>> What about splitting into tcg_out_cmp_imm and tcg_out_cmp_reg? or
>> tcg_out_cmp_i and tcg_out_cmp_r. The function is an 'if else' anyway with no
>> code sharing, and we would avoid sidestepping the TCGReg type check for b in
>> the _r case, as well as the const_b additional param.
> 
> This function is called once from tcg_out_tlb_read and three times from
> tcg_out_opc.  I just thought since the majority of uses would have to perform
> this if then we might as well have it in the subroutine than force all of the
> callers to replicate it.

Ok, that's a good point.
What about we keep the tcg_out_cmp and we have it do

if (const_b) {
   tcg_out_cmp_i();
} else {
   tcg_out_cmp_r();
}

so that code that wants to call cmp_r or cmp_i directly can do that?
I realize that at them moment it would benefit only tcg_out_tlb_read's use.

>>> +static void tcg_out_addsubi(TCGContext *s, int ext, TCGReg rd,
>>> +                            TCGReg rn, int aimm)
>>> +{
>>> +    AArch64Insn insn = INSN_ADDI;
>>> +    if (aimm < 0) {
>>> +        insn = INSN_SUBI;
>>> +        aimm = -aimm;
>>> +    }
>>> +    tcg_fmt_Rdn_aimm(s, insn, ext, rd, rn, aimm);
>>> +}
>>> +
>>
>> Could this be a tcg_out_arith_imm, in the similar way we would do
>> tcg_out_arith? (tcg_out_arith_reg?)
> 
> Which one gets renamed?  You already proposed tcg_fmt_Rdn_aimm be named
> tcg_out_arith_imm.  Now you want tcg_out_addsubi renamed to the same name?

This one confused me, possibly because I don't see the reason for addsubi.
> 
> I suppose we could merge the two if we add the S bit as a parameter.  Then
> we don't have to distinguish between ADDI and ADDIS, and we could share code
> with comparisons above...

I am ok with keeping the two separate, distinguishing in principle a 
subtraction from a comparison.

Claudio