Re: [Qemu-devel] [PATCH v2 5/8] target-mips: use DSP unions for binary DSP operators

[Blue Swirl]

On Sat, Jan 12, 2013 at 2:34 PM, Aurelien Jarno <address@hidden> wrote:
> On Sat, Jan 12, 2013 at 10:39:47AM +0000, Blue Swirl wrote:
>> On Thu, Jan 10, 2013 at 7:08 AM, Aurelien Jarno <address@hidden> wrote:
>> > On Wed, Jan 09, 2013 at 09:16:29PM +0000, Blue Swirl wrote:
>> >> On Wed, Jan 9, 2013 at 3:27 PM, Aurelien Jarno <address@hidden> wrote:
>> >> > This allow to reduce the number of macros.
>> >> >
>> >> > Signed-off-by: Aurelien Jarno <address@hidden>
>> >> > ---
>> >> >  target-mips/dsp_helper.c |  384 
>> >> > ++++++++++++++--------------------------------
>> >> >  1 file changed, 116 insertions(+), 268 deletions(-)
>> >> >
>> >> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
>> >> > index aed4c63..e01c8a9 100644
>> >> > --- a/target-mips/dsp_helper.c
>> >> > +++ b/target-mips/dsp_helper.c
>> >> > @@ -1078,7 +1078,6 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, 
>> >> > uint32_t b)
>> >> >          b = num & MIPSDSP_LO;               \
>> >> >      } while (0)
>> >> >
>> >> > -#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
>> >> >  #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
>> >> >                                           (((uint32_t)a << 24) | \
>> >> >                                           (((uint32_t)b << 16) | \
>> >> > @@ -1111,119 +1110,127 @@ static inline int32_t 
>> >> > mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>> >> >  #endif
>> >> >
>> >> >  /** DSP Arithmetic Sub-class insns **/
>> >> > -#define ARITH_PH(name, func)                                      \
>> >> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
>> >> > -{                                                                 \
>> >> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
>> >> > -                                                                  \
>> >> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
>> >> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
>> >> > -                                                                  \
>> >> > -    temph = mipsdsp_##func(rsh, rth);                             \
>> >> > -    templ = mipsdsp_##func(rsl, rtl);                             \
>> >> > -                                                                  \
>> >> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
>> >> > -}
>> >> > -
>> >> > -#define ARITH_PH_ENV(name, func)                                  \
>> >> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
>> >> > -                                CPUMIPSState *env)                \
>> >> > -{                                                                 \
>> >> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
>> >> > -                                                                  \
>> >> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
>> >> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
>> >> > -                                                                  \
>> >> > -    temph = mipsdsp_##func(rsh, rth, env);                        \
>> >> > -    templ = mipsdsp_##func(rsl, rtl, env);                        \
>> >> > -                                                                  \
>> >> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
>> >> > -}
>> >> > -
>> >> > -
>> >> > -ARITH_PH_ENV(addq, add_i16);
>> >> > -ARITH_PH_ENV(addq_s, sat_add_i16);
>> >> > -ARITH_PH_ENV(addu, add_u16);
>> >> > -ARITH_PH_ENV(addu_s, sat_add_u16);
>> >> > -
>> >> > -ARITH_PH(addqh, rshift1_add_q16);
>> >> > -ARITH_PH(addqh_r, rrshift1_add_q16);
>> >> > -
>> >> > -ARITH_PH_ENV(subq, sub_i16);
>> >> > -ARITH_PH_ENV(subq_s, sat16_sub);
>> >> > -ARITH_PH_ENV(subu, sub_u16_u16);
>> >> > -ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
>> >> > -
>> >> > -ARITH_PH(subqh, rshift1_sub_q16);
>> >> > -ARITH_PH(subqh_r, rrshift1_sub_q16);
>> >> > -
>> >> > -#undef ARITH_PH
>> >> > -#undef ARITH_PH_ENV
>> >> > +#define MIPSDSP32_BINOP(name, func, element)                           
>> >> >     \
>> >> > +target_ulong helper_##name(target_ulong rs, target_ulong rt)           
>> >> >     \
>> >> > +{                                                                      
>> >> >     \
>> >> > +    DSP32Value ds, dt;                                                 
>> >> >     \
>> >> > +    unsigned int i, n;                                                 
>> >> >     \
>> >> > +                                                                       
>> >> >     \
>> >> > +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                    
>> >> >     \
>> >> > +    ds.sw[0] = rs;                                                     
>> >> >     \
>> >> > +    dt.sw[0] = rt;                                                     
>> >> >     \
>> >> > +                                                                       
>> >> >     \
>> >> > +    for (i = 0 ; i < n ; i++) {                                        
>> >> >     \
>> >>
>> >> There's an extra space before ';', please remove. Also in the other
>> >> for loops below.
>> >
>> > It is not something I can find in CODING_STYLE, and it is also not
>> > caught by checkpatch.pl.
>>
>> No, but it's not common style by far:
>> egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
>> '--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
>> '--include=*.c' 'for.* ;' .|wc -l
>> 74
>> egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
>> '--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
>> '--include=*.c' 'for.*;' .|wc -l
>> 4585
>>
>> Original K&R style, from which QEMU style derives, didn't have the
>> spaces either. Perhaps you are influenced by French punctuation rules?
>
> I don't really care if it is common or not.

But commonality is the whole point of the CODING_STYLE and
checkpatch.pl, we want to drive the code base to be more uniform. If
having extra spaces was very common, like 25% or more of 'for'
statements, it would be clear that there is no general rule but in
this case it's clearly not.

> What I am saying is that if
> you want a rule to be enforced, it's better to at least have it written.
> It's also a good idea to have it added to checkpatch.pl, otherwise the
> benefit of this tool is greatly reduced.

I'm pretty sure there are many things that a simple Perl script which
only sees a few lines being patched can't ever catch and CODING_STYLE
or other documents can't also cover all aspects of how to make code.

In this case, a rule for avoiding space before a semicolon (probably
also colons too) could be possible. To avoid false positives, it
should be allowed where it's the only thing on a line. Also, cases
like this:
/src/qemu/hw/spapr_hcall.c:133:        for (i = 0; ; ++i) {
where the second statement in the for loop is not present, it makes
the text even clearer.

>
> --
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> address@hidden                 http://www.aurel32.net