[Qemu-devel] [PATCH v1 0/2] tcg: Add support for constant value promises

[Qemu-devel] [PATCH v1 0/2] tcg: Add support for constant value promises

[Lluís Vilanova]

A TCG constant value promise allows creating TCG code that works with a constant
whose value is not known until after the code has been generated (e.g., a count
of the instructions in a basic block).

This interface isolates the details of TCG code generation from the code that
needs this feature (e.g., in gen_icount).


Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---

Lluís Vilanova (2):
      tcg: Add support for constant value promises
      gen-icount: Use constant value promises


 include/exec/gen-icount.h |    8 +++-----
 tcg/tcg-op.h              |    6 ++++++
 tcg/tcg.c                 |   33 +++++++++++++++++++++++++++++++++
 tcg/tcg.h                 |   12 ++++++++++++
 4 files changed, 54 insertions(+), 5 deletions(-)


To: qemu-devel@nongnu.org
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Cc: peter.maydell@linaro.org
Cc: rth@twiddle.net

[Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Lluís Vilanova]

A TCG constant value promise allows creating TCG code that works with a
constant whose value is not known until after the code has been
generated (e.g., a count of the instructions in a basic block).

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 tcg/tcg-op.h |    6 ++++++
 tcg/tcg.c    |   33 +++++++++++++++++++++++++++++++++
 tcg/tcg.h    |   12 ++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index 4e20dc1..6966672 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -757,6 +757,7 @@ void tcg_gen_goto_tb(unsigned idx);
 
 #if TARGET_LONG_BITS == 32
 #define TCGv TCGv_i32
+#define TCGv_promise TCGv_promise_i32
 #define tcg_temp_new() tcg_temp_new_i32()
 #define tcg_global_reg_new tcg_global_reg_new_i32
 #define tcg_global_mem_new tcg_global_mem_new_i32
@@ -769,6 +770,7 @@ void tcg_gen_goto_tb(unsigned idx);
 #define tcg_gen_qemu_st_tl tcg_gen_qemu_st_i32
 #else
 #define TCGv TCGv_i64
+#define TCGv_promise TCGv_promise_i64
 #define tcg_temp_new() tcg_temp_new_i64()
 #define tcg_global_reg_new tcg_global_reg_new_i64
 #define tcg_global_mem_new tcg_global_mem_new_i64
@@ -914,6 +916,8 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index)
 #define tcg_gen_deposit_tl tcg_gen_deposit_i64
 #define tcg_const_tl tcg_const_i64
 #define tcg_const_local_tl tcg_const_local_i64
+#define tcg_promise_tl tcg_promise_i64
+#define tcg_set_promise_tl tcg_set_promise_i64
 #define tcg_gen_movcond_tl tcg_gen_movcond_i64
 #define tcg_gen_add2_tl tcg_gen_add2_i64
 #define tcg_gen_sub2_tl tcg_gen_sub2_i64
@@ -991,6 +995,8 @@ static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int mem_index)
 #define tcg_gen_deposit_tl tcg_gen_deposit_i32
 #define tcg_const_tl tcg_const_i32
 #define tcg_const_local_tl tcg_const_local_i32
+#define tcg_promise_tl tcg_promise_i32
+#define tcg_set_promise_tl tcg_set_promise_i32
 #define tcg_gen_movcond_tl tcg_gen_movcond_i32
 #define tcg_gen_add2_tl tcg_gen_add2_i32
 #define tcg_gen_sub2_tl tcg_gen_sub2_i32
diff --git a/tcg/tcg.c b/tcg/tcg.c
index a163541..ea5426d 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -691,6 +691,39 @@ TCGv_i64 tcg_const_local_i64(int64_t val)
     return t0;
 }
 
+TCGv_i32 tcg_promise_i32(TCGv_promise_i32 *promise)
+{
+    int pi = tcg_ctx.gen_next_parm_idx;
+    *promise = (TCGv_promise_i32)&tcg_ctx.gen_opparam_buf[pi];
+    return tcg_const_i32(0xdeadcafe);
+}
+
+TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
+{
+    int pi = tcg_ctx.gen_next_parm_idx;
+    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
+    return tcg_const_i64(0xdeadcafe);
+}
+
+void tcg_set_promise_i32(TCGv_promise_i32 promise, int32_t val)
+{
+    TCGArg *args = (TCGArg *)promise;
+    /* parameter as set by tcg_gen_op2() from tcg_gen_movi_i32() */
+    args[1] = val;
+}
+
+void tcg_set_promise_i64(TCGv_promise_i64 promise, int64_t val)
+{
+    TCGArg *args = (TCGArg *)promise;
+    /* parameter as set by tcg_gen_op2() from tcg_gen_movi_i64() */
+#if TCG_TARGET_REG_BITS == 32
+    args[1] = (int32_t)val;
+    args[3] = (int32_t)(val >> 32);
+#else
+    args[1] = val;
+#endif
+}
+
 #if defined(CONFIG_DEBUG_TCG)
 void tcg_clear_temp_count(void)
 {
diff --git a/tcg/tcg.h b/tcg/tcg.h
index a696922..79e83c8 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -308,6 +308,9 @@ typedef tcg_target_ulong TCGArg;
 typedef struct TCGv_i32_d *TCGv_i32;
 typedef struct TCGv_i64_d *TCGv_i64;
 typedef struct TCGv_ptr_d *TCGv_ptr;
+typedef struct TCGv_promise_i32_d *TCGv_promise_i32;
+typedef struct TCGv_promise_i64_d *TCGv_promise_i64;
+typedef struct TCGv_promise_ptr_d *TCGv_promise_ptr;
 
 static inline TCGv_i32 QEMU_ARTIFICIAL MAKE_TCGV_I32(intptr_t i)
 {
@@ -746,6 +749,8 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdefs);
 #define TCGV_PTR_TO_NAT(n) MAKE_TCGV_I32(GET_TCGV_PTR(n))
 
 #define tcg_const_ptr(V) TCGV_NAT_TO_PTR(tcg_const_i32((intptr_t)(V)))
+#define tcg_promise_ptr(V) TCGV_NAT_TO_PTR(tcg_promise_i32((intptr_t)(V)))
+#define tcg_set_promise_ptr(V) TCGV_NAT_TO_PTR(tcg_set_promise_i32((intptr_t)(V)))
 #define tcg_global_reg_new_ptr(R, N) \
     TCGV_NAT_TO_PTR(tcg_global_reg_new_i32((R), (N)))
 #define tcg_global_mem_new_ptr(R, O, N) \
@@ -757,6 +762,8 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdefs);
 #define TCGV_PTR_TO_NAT(n) MAKE_TCGV_I64(GET_TCGV_PTR(n))
 
 #define tcg_const_ptr(V) TCGV_NAT_TO_PTR(tcg_const_i64((intptr_t)(V)))
+#define tcg_promise_ptr(V) TCGV_NAT_TO_PTR(tcg_promise_i64((intptr_t)(V)))
+#define tcg_set_promise_ptr(P, V) tcg_set_promise_i64(P, (intptr_t)(V)))
 #define tcg_global_reg_new_ptr(R, N) \
     TCGV_NAT_TO_PTR(tcg_global_reg_new_i64((R), (N)))
 #define tcg_global_mem_new_ptr(R, O, N) \
@@ -780,6 +787,11 @@ TCGv_i64 tcg_const_i64(int64_t val);
 TCGv_i32 tcg_const_local_i32(int32_t val);
 TCGv_i64 tcg_const_local_i64(int64_t val);
 
+TCGv_i32 tcg_promise_i32(TCGv_promise_i32 *promise);
+TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise);
+void tcg_set_promise_i32(TCGv_promise_i32 promise, int32_t val);
+void tcg_set_promise_i64(TCGv_promise_i64 promise, int64_t val);
+
 TCGLabel *gen_new_label(void);
 
 /**

Re: [Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Richard Henderson]

On 01/15/2016 07:35 AM, Lluís Vilanova wrote:
> +TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
> +{
> +    int pi = tcg_ctx.gen_next_parm_idx;
> +    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
> +    return tcg_const_i64(0xdeadcafe);
> +}

This doesn't work for a 32-bit host.  The constant may be split across two
different parameter indices, and you don't know exactly where the second will be.

Because of that, I think this is over-engineered, and really prefer the simpler
interface that Edgar posted last week.



r~

Re: [Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Lluís Vilanova]

Richard Henderson writes:

> On 01/15/2016 07:35 AM, Lluís Vilanova wrote:
>> +TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
>> +{
>> +    int pi = tcg_ctx.gen_next_parm_idx;
>> +    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
>> +    return tcg_const_i64(0xdeadcafe);
>> +}

> This doesn't work for a 32-bit host.  The constant may be split across two
> different parameter indices, and you don't know exactly where the second will be.

> Because of that, I think this is over-engineered, and really prefer the simpler
> interface that Edgar posted last week.

In this case, 'tcg_set_promise_i64' sets the two arguments accordingly on 32-bit
targets. Both solutions depend on TCG internals (in this specific case the
implementation of 'tcg_gen_movi_i64'), but now it's all implemented inside TCG.

Alternatively, promises could use the longer route of recording the opcode index
(as Edgar did AFAIR), and retrieve the argument pointer from there. Still, for
32-bit targets we have to assume the two immediate moves are gonna generate two
consecutive opcodes.


Thanks,
  Lluis

Re: [Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Richard Henderson]

On 01/15/2016 12:12 PM, Lluís Vilanova wrote:
> Richard Henderson writes:
> 
>> On 01/15/2016 07:35 AM, Lluís Vilanova wrote:
>>> +TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
>>> +{
>>> +    int pi = tcg_ctx.gen_next_parm_idx;
>>> +    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
>>> +    return tcg_const_i64(0xdeadcafe);
>>> +}
> 
>> This doesn't work for a 32-bit host.  The constant may be split across two
>> different parameter indices, and you don't know exactly where the second will be.
> 
>> Because of that, I think this is over-engineered, and really prefer the simpler
>> interface that Edgar posted last week.
> 
> In this case, 'tcg_set_promise_i64' sets the two arguments accordingly on 32-bit
> targets. Both solutions depend on TCG internals (in this specific case the
> implementation of 'tcg_gen_movi_i64'), but now it's all implemented inside TCG.
> 
> Alternatively, promises could use the longer route of recording the opcode index
> (as Edgar did AFAIR), and retrieve the argument pointer from there. Still, for
> 32-bit targets we have to assume the two immediate moves are gonna generate two
> consecutive opcodes.

Your solution also doesn't help Edgar, since he's interested in modifying an
argument to the insn_start opcode, not modifying a literal constant in a move.



r~

Re: [Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Lluís Vilanova]

Richard Henderson writes:

> On 01/15/2016 12:12 PM, Lluís Vilanova wrote:
>> Richard Henderson writes:
>> 
>>> On 01/15/2016 07:35 AM, Lluís Vilanova wrote:
>>>> +TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
>>>> +{
>>>> +    int pi = tcg_ctx.gen_next_parm_idx;
>>>> +    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
>>>> +    return tcg_const_i64(0xdeadcafe);
>>>> +}
>> 
>>> This doesn't work for a 32-bit host.  The constant may be split across two
>>> different parameter indices, and you don't know exactly where the second will be.
>> 
>>> Because of that, I think this is over-engineered, and really prefer the simpler
>>> interface that Edgar posted last week.
>> 
>> In this case, 'tcg_set_promise_i64' sets the two arguments accordingly on 32-bit
>> targets. Both solutions depend on TCG internals (in this specific case the
>> implementation of 'tcg_gen_movi_i64'), but now it's all implemented inside TCG.
>> 
>> Alternatively, promises could use the longer route of recording the opcode index
>> (as Edgar did AFAIR), and retrieve the argument pointer from there. Still, for
>> 32-bit targets we have to assume the two immediate moves are gonna generate two
>> consecutive opcodes.

> Your solution also doesn't help Edgar, since he's interested in modifying an
> argument to the insn_start opcode, not modifying a literal constant in a move.

I wasn't aware of that. If the idea was to use this for more than immediates
stored in TCGv values, I see two options. First, modify the necessary opcodes to
use a TCGv argument instead of an immediate. Second, generalize this patch to
to select any opcode argument.

An example of the generalization when used to reimplement icount:

   // insn count placeholder
   TCGv_i32 imm = tcg_const_i32(0xcafecafe);
   // insn count promise
   TCGv_promise_i32 imm_promise = tcg_promise_i32(
       1,  // how many opcodes to go "backwards"
       1); // what argument to modify on that opcode
   // operate with imm
   ...
   // resolve value
   tcg_set_promise_i32(imm_promise, insn_count);

The question still stands on how to cleanly handle promises for opcodes like a
64-bit mov on a 32-bit host (it's generated as two opcodes). Using this
interface would still be cleaner than directly manipulating the low-level TCG
arrays, and makes it easier to adopt it in future changes.


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Edgar E. Iglesias]

On Sat, Jan 16, 2016 at 09:57:36PM +0100, Lluís Vilanova wrote:
> Richard Henderson writes:
> 
> > On 01/15/2016 12:12 PM, Lluís Vilanova wrote:
> >> Richard Henderson writes:
> >> 
> >>> On 01/15/2016 07:35 AM, Lluís Vilanova wrote:
> >>>> +TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
> >>>> +{
> >>>> +    int pi = tcg_ctx.gen_next_parm_idx;
> >>>> +    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
> >>>> +    return tcg_const_i64(0xdeadcafe);
> >>>> +}
> >> 
> >>> This doesn't work for a 32-bit host.  The constant may be split across two
> >>> different parameter indices, and you don't know exactly where the second will be.
> >> 
> >>> Because of that, I think this is over-engineered, and really prefer the simpler
> >>> interface that Edgar posted last week.
> >> 
> >> In this case, 'tcg_set_promise_i64' sets the two arguments accordingly on 32-bit
> >> targets. Both solutions depend on TCG internals (in this specific case the
> >> implementation of 'tcg_gen_movi_i64'), but now it's all implemented inside TCG.
> >> 
> >> Alternatively, promises could use the longer route of recording the opcode index
> >> (as Edgar did AFAIR), and retrieve the argument pointer from there. Still, for
> >> 32-bit targets we have to assume the two immediate moves are gonna generate two
> >> consecutive opcodes.
> 
> > Your solution also doesn't help Edgar, since he's interested in modifying an
> > argument to the insn_start opcode, not modifying a literal constant in a move.
> 
> I wasn't aware of that. If the idea was to use this for more than immediates
> stored in TCGv values, I see two options. First, modify the necessary opcodes to
> use a TCGv argument instead of an immediate. Second, generalize this patch to
> to select any opcode argument.
> 
> An example of the generalization when used to reimplement icount:
> 
>    // insn count placeholder
>    TCGv_i32 imm = tcg_const_i32(0xcafecafe);
>    // insn count promise
>    TCGv_promise_i32 imm_promise = tcg_promise_i32(
>        1,  // how many opcodes to go "backwards"
>        1); // what argument to modify on that opcode
>    // operate with imm
>    ...
>    // resolve value
>    tcg_set_promise_i32(imm_promise, insn_count);
> 
> The question still stands on how to cleanly handle promises for opcodes like a
> 64-bit mov on a 32-bit host (it's generated as two opcodes). Using this
> interface would still be cleaner than directly manipulating the low-level TCG
> arrays, and makes it easier to adopt it in future changes.
>

Thanks Lluis and Richard,

I'll stay with my version for the first try at the ARM load/store fault
reporting. If something better comes along that works for me, I'm happy
to change.

Richard if you want to take the patches through your tree feel free to
do so. Otherwise, I'll post them again with more context and try through
the ARM queue.

Best regards,
Edgar 

Re: [Qemu-devel] [PATCH v1 1/2] tcg: Add support for constant value promises

[Lluís Vilanova]

Edgar E Iglesias writes:

> On Sat, Jan 16, 2016 at 09:57:36PM +0100, Lluís Vilanova wrote:
>> Richard Henderson writes:
>> 
>> > On 01/15/2016 12:12 PM, Lluís Vilanova wrote:
>> >> Richard Henderson writes:
>> >> 
>> >>> On 01/15/2016 07:35 AM, Lluís Vilanova wrote:
>> >>>> +TCGv_i64 tcg_promise_i64(TCGv_promise_i64 *promise)
>> >>>> +{
>> >>>> +    int pi = tcg_ctx.gen_next_parm_idx;
>> >>>> +    *promise = (TCGv_promise_i64)&tcg_ctx.gen_opparam_buf[pi];
>> >>>> +    return tcg_const_i64(0xdeadcafe);
>> >>>> +}
>> >> 
>> >>> This doesn't work for a 32-bit host.  The constant may be split across two
>> >>> different parameter indices, and you don't know exactly where the second will be.
>> >> 
>> >>> Because of that, I think this is over-engineered, and really prefer the simpler
>> >>> interface that Edgar posted last week.
>> >> 
>> >> In this case, 'tcg_set_promise_i64' sets the two arguments accordingly on 32-bit
>> >> targets. Both solutions depend on TCG internals (in this specific case the
>> >> implementation of 'tcg_gen_movi_i64'), but now it's all implemented inside TCG.
>> >> 
>> >> Alternatively, promises could use the longer route of recording the opcode index
>> >> (as Edgar did AFAIR), and retrieve the argument pointer from there. Still, for
>> >> 32-bit targets we have to assume the two immediate moves are gonna generate two
>> >> consecutive opcodes.
>> 
>> > Your solution also doesn't help Edgar, since he's interested in modifying an
>> > argument to the insn_start opcode, not modifying a literal constant in a move.
>> 
>> I wasn't aware of that. If the idea was to use this for more than immediates
>> stored in TCGv values, I see two options. First, modify the necessary opcodes to
>> use a TCGv argument instead of an immediate. Second, generalize this patch to
>> to select any opcode argument.
>> 
>> An example of the generalization when used to reimplement icount:
>> 
>> // insn count placeholder
>> TCGv_i32 imm = tcg_const_i32(0xcafecafe);
>> // insn count promise
>> TCGv_promise_i32 imm_promise = tcg_promise_i32(
>> 1,  // how many opcodes to go "backwards"
>> 1); // what argument to modify on that opcode
>> // operate with imm
>> ...
>> // resolve value
>> tcg_set_promise_i32(imm_promise, insn_count);
>> 
>> The question still stands on how to cleanly handle promises for opcodes like a
>> 64-bit mov on a 32-bit host (it's generated as two opcodes). Using this
>> interface would still be cleaner than directly manipulating the low-level TCG
>> arrays, and makes it easier to adopt it in future changes.
>> 

> Thanks Lluis and Richard,

> I'll stay with my version for the first try at the ARM load/store fault
> reporting. If something better comes along that works for me, I'm happy
> to change.

> Richard if you want to take the patches through your tree feel free to
> do so. Otherwise, I'll post them again with more context and try through
> the ARM queue.

My offer still stands. If the generalized interface seems adequate (specific
opcode argument to set the promise for), it's a rather simple change on the
series.


Cheers,
  Lluis

[Qemu-devel] [PATCH v1 2/2] gen-icount: Use constant value promises

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/exec/gen-icount.h |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 05d89d3..98d7a85 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -5,7 +5,7 @@
 
 /* Helpers for instruction counting code generation.  */
 
-static TCGArg *icount_arg;
+static TCGv_promise_i32 icount_promise;
 static TCGLabel *icount_label;
 static TCGLabel *exitreq_label;
 
@@ -30,13 +30,11 @@ static inline void gen_tb_start(TranslationBlock *tb)
     tcg_gen_ld_i32(count, cpu_env,
                    -ENV_OFFSET + offsetof(CPUState, icount_decr.u32));
 
-    imm = tcg_temp_new_i32();
-    tcg_gen_movi_i32(imm, 0xdeadbeef);
+    imm = tcg_promise_i32(&icount_promise);
 
     /* This is a horrid hack to allow fixing up the value later.  */
     i = tcg_ctx.gen_last_op_idx;
     i = tcg_ctx.gen_op_buf[i].args;
-    icount_arg = &tcg_ctx.gen_opparam_buf[i + 1];
 
     tcg_gen_sub_i32(count, count, imm);
     tcg_temp_free_i32(imm);
@@ -53,7 +51,7 @@ static void gen_tb_end(TranslationBlock *tb, int num_insns)
     tcg_gen_exit_tb((uintptr_t)tb + TB_EXIT_REQUESTED);
 
     if (tb->cflags & CF_USE_ICOUNT) {
-        *icount_arg = num_insns;
+        tcg_set_promise_i32(icount_promise, num_insns);
         gen_set_label(icount_label);
         tcg_gen_exit_tb((uintptr_t)tb + TB_EXIT_ICOUNT_EXPIRED);
     }