Re: [PATCH 1/4] target/arm: Add TB flag for "MVE insns not predicated"

[Peter Maydell]

On Fri, 3 Sept 2021 at 16:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/3/21 4:17 PM, Peter Maydell wrote:
> > I don't really understand what you mean here. What's the difference
> > between ending the TB now and translating a few more insns in
> > this TB before we end it?
>
>    VCMP (pred now on or off)
>    B    label
>
> The code we emit for B uses goto_tb.
>
> When goto_tb is unlinked (e.g. first execute), we exit with a code that 
> causes the current
> cpu state to be evaluated, the new TB created, and the link filled in.  This 
> makes
> permanent the cpu state we evaluated, and thus the state change across 
> goto_tb must be
> constant.  This is fine for B, since only the PC changes and to a known 
> destination.
>
> So we have two choices: end the TB immediately after the VCMP, or set another 
> flag to
> cause gen_goto_tb to fall back to goto_ptr.

Ah, I see, and when we end after the VCMP then we use UPDATE_NOCHAIN
which causes us to use lookup_and_goto_ptr instead of goto_tb.

I was wondering if this is also a problem for the flags like s->v7m_lspact,
which we currently handle in about this way -- set from a TB flag, but
then updated for the rest of the TB. Let me try to write out the rules:

 * if you do something that changes the TB flag, and you know for
   definite the new state, and this change happens and is the same
   for every exit from the TB, then you can just update the flag
   and keep going in this TB. (This is why lspact etc are OK)
 * if you don't know for definite the new state, because it might
   not have happened, eg a helper function condition changes
   something, then you must end the TB immediately (in a way that
   doesn't use goto_tb). This is true both for "I couldn't figure
   out the new state, it's too hard" and "the new state depends on
   some runtime data such that different executions of the same TB
   might end up with different values for the flag"
 * it's not good enough to say "well, I can pessimistically always
   assume mve_no_pred", because of the goto_tb issue. You have to
   be sure of the exact value that a TB flags calculation after
   the insn will get.

So I think that pretty much everywhere in my current patch that is setting
s->mve_no_pred = false needs instead to just end the TB. That seems mostly
straightforward, but there are some tricky cases:

 * WLSTP. The code generated for this insn has two exits. The change
   to ltpsize happens on only one of those, which is currently
   handled by gen_jmp_tb(). I think we are OK to just leave the
   code as it is, because the value we pass to LTPSIZE is
   constant (encoded in the instruction), and so the new value
   of MVE_NO_PRED is always the same on that exit from the TB
   for all executions
 * gen_update_fp_context() -- this function gets called for pretty
   much every FP/MVE insn (as part of vfp_access_check), and it
   can in rare cases update the FPSCR.LTPSIZE and the VPR. I guess
   this means we really do need to end the TB
   if (MVE && s->v7m_new_fp_ctxt_needed) (ie the comment in
   gen_update_fp_context "We don't need to arrange to end the TB,
   because [we don't cache FPSCR in TB flags]" is no longer true).
   That seems likely to be painful because some of the insns that
   do a vfp_access_check do complicated things with the TB exits
   (eg WLSTP, LETP) that are going to override a naive setting of
   is_jmp in gen_update_fp_context()...
 * gen_preserve_fp_state() is similar to gen_update_fp_context()

thanks
-- PMM