Re: [Qemu-devel] [PATCH V3 01/12] add MIPS DSP internal functions

[Jia Liu]

On Tue, Mar 27, 2012 at 11:33 PM, Richard Henderson <address@hidden> wrote:
> On 03/27/12 02:24, Jia Liu wrote:
>> +ifeq ($(TARGET_BASE_ARCH), mips)
>> +libobj-y += dsp_helper.o
>> +endif
> ...
>
>> +#include "dyngen-exec.h"
>> +
>> +/*** MIPS DSP internal functions begin ***/
>> +static inline void set_DSPControl_overflow_flag(uint32_t flag, int position)
>> +{
>> +    env->active_tc.DSPControl |= (target_ulong)flag << position;
>> +}
>
> In order to use dyngen-exec.h, and the global ENV variable,
> you must also do
>
> dsp_helper.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
>
> in the Makefile.target.  If you had tested i386 host instead
> of x86_64, you should have seen this failure.
>
> Better, from the point of view of one working toward eliminating
> dyngen-exec.h and these special calling conventions, is to actually
> pass down env in the helper.  E.g.
>

Thanks, I'll add it.

> +/** DSP Arithmetic Sub-class insns **/
> +uint32_t helper_addq_ph(CPUMIPSState *env, uint32_t rs, uint32_t rt)
>
> +DEF_HELPER_FLAGS_3(addq_ph, 0, i32, env, i32, i32)
>
> where mipsdsp_add_i16 and thence set_DSPControl_overflow_flag also
> grow ENV parameters.
>
> In addition, we can't use TCG_CALL_CONST | TCG_CALL_PURE on any
> function that sets DSPControl, because that's the cpu_dspctrl tcg
> variable.
>
> You might get better performance out of TCG if you do *not* make
> DSPControl a TCG variable, but instead perform explicit loads from
> that slot in the few places that need it.  That way you can at
> least set TCG_CALL_CONST.  (Though not TCG_CALL_PURE, because the
> functions do have side effects.)
>

Thanks.
do you mean, I should write like this?
helper.h:
DEF_HELPER_FLAGS_3(addq_ph, 0, i32, env, i32, i32)

dsp_helper.c:
uint32_t helper_addq_ph(CPUMIPSState *env, uint32_t rs, uint32_t rt)
{}

Sorry I think I'm not turly understand your comment.


>> +    if (len == 2)
>> +        env->active_tc.DSPControl &= 0xFCFFFFFF;
>> +    else if (len == 4)
>> +        env->active_tc.DSPControl &= 0xF0FFFFFF;
>
> Run all your patches through ./scripts/checkpatch.pl and fix the
> errors that will report.
>

I've run ./scripts/checkpatch.pl, but I didn't get a ERR here...

It do have some ERR, but the all the other Macros write like that, so
I didn't have a idea about that.

>
> r~

Regards,
Jia.