Re: [PATCH 2/3] Replace `TARGET_TB_PCREL` with `CF_PCREL`

[Richard Henderson]

On 2/7/23 00:43, Anton Johansson wrote:
diff --git a/accel/tcg/tb-jmp-cache.h b/accel/tcg/tb-jmp-cache.h
index b3f6e78835..083939b302 100644
--- a/accel/tcg/tb-jmp-cache.h
+++ b/accel/tcg/tb-jmp-cache.h
  static inline TranslationBlock *
-tb_jmp_cache_get_tb(CPUJumpCache *jc, uint32_t hash)
+tb_jmp_cache_get_tb(CPUJumpCache *jc, uint32_t cflags, uint32_t hash)
  {
-#if TARGET_TB_PCREL
-    /* Use acquire to ensure current load of pc from jc. */
-    return qatomic_load_acquire(&jc->array[hash].tb);
-#else
-    /* Use rcu_read to ensure current load of pc from *tb. */
-    return qatomic_rcu_read(&jc->array[hash].tb);
-#endif
+    if (cflags & CF_PCREL) {
+        /* Use acquire to ensure current load of pc from jc. */
+        return qatomic_load_acquire(&jc->array[hash].tb);
+    } else {
+        /* Use rcu_read to ensure current load of pc from *tb. */
+        return qatomic_rcu_read(&jc->array[hash].tb);
+    }
  }
  
  static inline target_ulong
  tb_jmp_cache_get_pc(CPUJumpCache *jc, uint32_t hash, TranslationBlock *tb)
  {
-#if TARGET_TB_PCREL
-    return jc->array[hash].pc;
-#else
-    return tb_pc(tb);
-#endif
+    if (tb_cflags(tb) & CF_PCREL) {
+        return jc->array[hash].pc;
+    } else {
+        return tb_pc(tb);
+    }
  }
  
  static inline void
  tb_jmp_cache_set(CPUJumpCache *jc, uint32_t hash,
                   TranslationBlock *tb, target_ulong pc)
  {
-#if TARGET_TB_PCREL
-    jc->array[hash].pc = pc;
-    /* Use store_release on tb to ensure pc is written first. */
-    qatomic_store_release(&jc->array[hash].tb, tb);
-#else
-    /* Use the pc value already stored in tb->pc. */
-    qatomic_set(&jc->array[hash].tb, tb);
-#endif
+    if (tb_cflags(tb) & CF_PCREL) {
+        jc->array[hash].pc = pc;
+        /* Use store_release on tb to ensure pc is written first. */
+        qatomic_store_release(&jc->array[hash].tb, tb);
+    } else{
+        /* Use the pc value already stored in tb->pc. */
+        qatomic_set(&jc->array[hash].tb, tb);
+    }
  }

These little functions made sense when they were isolating ifdefs.
When they protect a sequence of conditions,

    if (CF_PCREL) {
        a
    } else {
        b
    }
    if (CF_PCREL) {
        c
    } else {
        d
    }
    if (CF_PCREL) {
        e
    } else {
        f
    }

we probably want to hoist one check in the callers.

+    return ((tb_cflags(a) & CF_PCREL || tb_pc(a) == tb_pc(b)) &&

Similarly things like this, where we have a PCREL test here, and also within 
tb_pc().

-    h = tb_hash_func(phys_pc, (TARGET_TB_PCREL ? 0 : tb_pc(tb)),
+    h = tb_hash_func(phys_pc, (orig_cflags & CF_PCREL ? 0 : tb_pc(tb)),

etc.

This does too much at once, and also disables TARGET_TB_PCREL for one patch, changing 
behaviour during bisection.


r~