Re: [Qemu-arm] [PATCH 07/67] target/arm: Introduce add_reg_for_lit

[Peter Maydell]

On Tue, 30 Jul 2019 at 01:51, Richard Henderson
<address@hidden> wrote:
>
> On 7/29/19 7:15 AM, Peter Maydell wrote:
> > On Fri, 26 Jul 2019 at 18:50, Richard Henderson
> > <address@hidden> wrote:
> >>
> >> Used only on the thumb side so far, but will be more obvious
> >> once we start unifying the implementation of A32+T32.
> >>
> >> Signed-off-by: Richard Henderson <address@hidden>
> >> ---

> > This is losing the information in the comments about the UNPREDICTABLE
> > cases. Are there callsites where the new function is called where
> > "thumb and reg == 15" is not UNPREDICTABLE, or are they all
> > that way?
>
> These call sites are that way, but this function will eventually be used for
> LDR (literal) and ADR, which obviously are not UNPREDICTABLE.
>
> I don't think this comment attached to this code is useful as-is.  Either we 
> do
> the natural a32-ish behaviour and use ALIGN(PC,4), or we should
> gen_illegal_op() and be done with it.

I think it's usually worth noting when something's UNPREDICTABLE
and we're choosing to take the falls-out-of-the-code behaviour,
that's all.

> Would you prefer a function like
>
> /* Use of PC is UNPREDICTABLE in thumb mode, but allowed in arm mode. */
> static TCGv_i32 load_reg_nothumbpc(DisasContext *s, int reg)
> {
>     if (unlikely(reg == 15) && s->thumb) {
>         gen_illegal_op(s);
>         /* Unreachable tcg ops will be deleted but must still be legal. */
>         return tcg_const_i32(0);
>     }
>     return load_reg(s, reg);
> }
>
> for these specific usages?

I definitely don't favour this -- all our "is this going to UNDEF"
checks should go right at the start before we generate any
TCG code at all for the insn. One of the things I'm hoping this
series cleans up is that the current decoder is quite bad at
sometimes detecting UNDEF conditions late (which then results
in warnings about potential leaks of TCG variables).

thanks
-- PMM