Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation

[Richard Henderson <richard.henderson@linaro.org>]

On 3/13/21 6:40 PM, Taylor Simpson wrote:
> 
> 
>> -----Original Message-----
>> From: Richard Henderson <richard.henderson@linaro.org>
>> Sent: Sunday, February 14, 2021 7:04 PM
>> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
>> Cc: philmd@redhat.com; alex.bennee@linaro.org; laurent@vivier.eu;
>> ale@rev.ng; Brian Cain <bcain@quicinc.com>
>> Subject: Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation
>>
>> On 2/7/21 9:46 PM, Taylor Simpson wrote:
>>> +static inline void ctx_log_reg_write(DisasContext *ctx, int rnum)
>>
>> Drop the inline markup throughout.
> 
> I can go through the code and remove unnecessary inline's.  However, these particular inline's are needed because this is a header file.  If we remove the inline and the header gets included in a .c file that doesn't use the function, we get a "defined but not used" error.  Also, we need to keep the inline's in genptr.c to avoid the same error when we switch an instruction between the fGEN_TCG and helper implementations (and the idef-parser in the future).  Also, there is one function that needs to be inline for performance reasons.  I'll add a comment for that one.
> 
>>> +        words[nwords] = cpu_ldl_code(env,
>>> +                                ctx->base.pc_next + nwords * sizeof(uint32_t));
>>
>> translate_ldl, so that a plugin has access to the packet data.  (Note that
>> pkt_crosses_page is fine, because that's read-ahead, not reads for the
>> current
>> packet.)
> 
> OK
> 
>>
>> Fold this to a simple function call:
>>
>> static void gen_check_store_width(...)
>> {
>>      if (HEX_DEBUG) {
>>         ....
>>      }
>> }
> 
> OK
> 
>>> +#if HEX_DEBUG
>>> +        /* When debugging, only put one packet per TB */
>>> +        ctx->base.is_jmp = DISAS_TOO_MANY;
>>> +#endif
>>
>> Why?  You can always add -singlestep to the command-line.
> 
> OK
> 
>>> +    case DISAS_NORETURN:
>>> +        gen_exec_counters(ctx);
>>> +        tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], hex_next_PC);
>>> +        if (ctx->base.singlestep_enabled) {
>>> +            gen_exception_debug();
>>> +        } else {
>>> +            tcg_gen_exit_tb(NULL, 0);
>>> +        }
>>
>> DISAS_NORETURN says that we have *already* exited the TB.  None of the
>> code you
>> emit here will be reachable.
> 
> Isn't this called before the TB ends?

Yes, but DISAS_NORETURN still means we've already exited.

Just like calling abort() in C means that we won't reach any following return 
statement.


r~