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
