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

[Richard Henderson]

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.


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

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


r~