Re: [Qemu-devel] [PATCH v3 18/25] tcg: Add TCG_MAX_INSNS

[Richard Henderson <rth@twiddle.net>]

On 09/24/2015 01:02 PM, Aurelien Jarno wrote:
>> @@ -2903,6 +2903,9 @@ static inline void gen_intermediate_code_internal(AlphaCPU *cpu,
>>      if (max_insns == 0) {
>>          max_insns = CF_COUNT_MASK;
>>      }
> 
> I guess you can change also change the value to TCG_MAX_INSNS, though I
> guess the compiler will realize about that.

I did wonder about the best thing to do re CF_COUNT_MASK.  Especially as it's
currently set to 0x7fff.  FWIW, the largest TB I've seen so far while
collecting statistics is 157 insns.  So the current setting of TCG_MAX_INSNS at
512 is more than enough.

> 
>> +    if (max_insns > TCG_MAX_INSNS) {
>> +        max_insns = TCG_MAX_INSNS;
>> +    }
>>  
>>      if (in_superpage(&ctx, pc_start)) {
>>          pc_mask = (1ULL << 41) - 1;
> 
> Given we have the same pattern in all targets, I do wonder if it
> wouldn't be better to just setup (cflags & CF_COUNT_MASK) to
> TCG_MAX_INSNS instead of 0 in translate-all.c when not using icount.

Yes, that would probably be best.

There should probably be some helper function that handles all these as well as
noticing single-stepping.  Too many targets test

   (num_insns >= max_insns || singlestep || ...)

when we could just as well set max_insns to 1 and have just the one runtime
test.  Then there's all the targets which have a fixed insn size, where we can
pre-compute the number of insns left on the page, and fold in the end-of-page
test as well.

I'll put cleaning this up on the to-do list.


r~

> 
> That said your code is correct, so:
> 
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
>