Re: [PATCH 08/13] target/i386: make cc_op handling more explicit for repeated string instructions.

[Richard Henderson]

On 12/15/24 09:13, Paolo Bonzini wrote:
> Il dom 15 dic 2024, 15:45 Richard Henderson <richard.henderson@linaro.org 
> <mailto:richard.henderson@linaro.org>> ha scritto:
>
>     On 12/15/24 03:06, Paolo Bonzini wrote:
>      >       fn(s, ot);
>      >       gen_op_add_reg_im(s, s->aflag, R_ECX, -1);
>      > +    gen_update_cc_op(s);
>      > +
>      > +    /* Leave if REP condition fails.  */
>      >       if (is_repz_nz) {
>      >           int nz = (s->prefix & PREFIX_REPNZ) ? 1 : 0;
>      > -        gen_jcc(s, (JCC_Z << 1) | (nz ^ 1), done);
>      > +        gen_jcc_noeob(s, (JCC_Z << 1) | (nz ^ 1), done);
>
>     The comment in gen_jcc would still seem to apply:
>
>           CCPrepare cc = gen_prepare_cc(s, b, NULL);
>
>           /*
>            * Note that this must be _after_ gen_prepare_cc, because it
>            * can change the cc_op from CC_OP_DYNAMIC to CC_OP_EFLAGS!
>            */
>           gen_update_cc_op(s);
>
>     via any path through gen_prepare_cc that reaches gen_compute_eflags.
>
>     However!  Because this is JCC_Z, we will never call gen_compute_eflags, we 
> will always go
>     through the gen_prepare_eflags_z, which doesn't have the same problem.
>
>
> Or more simply, fn(s, ot) must have left CC_OP_SUBx in cc_op.

No, even CC_OP_SUBx can (and in this case, will) goto slow_jcc.
My correctness analysis is strictly based on JCC_Z.

>     This subtlety deserves a comment and maybe an assert.  Perhaps
>
>           gen_jcc_noeob(...);
>           assert(!s->cc_op_dirty);
>
>
> Either that or an assert(s->cc_op != CC_OP_DYNAMIC) before the call to 
> gen_jcc_noeob().

No, not before gen_jcc_noeob, since that's where any buggy change would occur.

r~