Re: [PATCH v2 05/25] target/i386: cleanup cc_op changes for REP/REPZ/REPNZ

[Richard Henderson]

On 5/6/24 01:09, Paolo Bonzini wrote:
gen_update_cc_op must be called before control flow splits.  Do it
where the jump on ECX!=0 is translated.
On the other hand, remove the call before gen_jcc1, which takes care of
it already, and explain why REPZ/REPNZ need not use CC_OP_DYNAMIC---the
translation block ends before any control-flow-dependent cc_op could
be observed.

Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
  target/i386/tcg/translate.c | 13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 3f1d2858fc9..6b766f5dd3f 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1242,11 +1242,15 @@ static inline void gen_jcc1(DisasContext *s, int b, 
TCGLabel *l1)
  }
  
  /* XXX: does not work with gdbstub "ice" single step - not a
-   serious problem */
+   serious problem.  The caller can jump to the returned label
+   to stop the REP but, if the flags have changed, it has to call
+   gen_update_cc_op before doing so.  */
  static TCGLabel *gen_jz_ecx_string(DisasContext *s)
  {
      TCGLabel *l1 = gen_new_label();
      TCGLabel *l2 = gen_new_label();
+
+    gen_update_cc_op(s);
      gen_op_jnz_ecx(s, l1);
      gen_set_label(l2);
      gen_jmp_rel_csize(s, 0, 1);
@@ -1342,7 +1346,6 @@ static void gen_repz(DisasContext *s, MemOp ot,
                       void (*fn)(DisasContext *s, MemOp ot))
  {
      TCGLabel *l2;
-    gen_update_cc_op(s);
      l2 = gen_jz_ecx_string(s);
      fn(s, ot);
      gen_op_add_reg_im(s, s->aflag, R_ECX, -1);

Ok.


@@ -1364,11 +1367,13 @@ static void gen_repz2(DisasContext *s, MemOp ot, int nz,
                        void (*fn)(DisasContext *s, MemOp ot))
  {
      TCGLabel *l2;
-    gen_update_cc_op(s);
      l2 = gen_jz_ecx_string(s);
+    /*
+     * Only one iteration is done at a time, so there is
+     * no control flow junction here and cc_op is never dynamic.
+     */
      fn(s, ot);
      gen_op_add_reg_im(s, s->aflag, R_ECX, -1);
-    gen_update_cc_op(s);
      gen_jcc1(s, (JCC_Z << 1) | (nz ^ 1), l2);
      if (s->repz_opt) {
          gen_op_jz_ecx(s, l2);

Ok, but only because gen_jcc1 does the gen_update_cc_op.  The comment is neither correct 
nor necessary.

The reason to write cc_op before branches instead of junctions is to avoid having *two* 
writes of cc_op on either side of the branch.


r~