[Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

On AArch64, when some load/stores trap under specific conditions, a set of
detailed info describing the insn is provided to the trap handler (e.g size
of the access, target registers, insn-length mode etc).
This specific info is known at translation time and Peter suggested that
we have a look at the insn_start mechanism to see if we can reuse it
to pass along the info to the exception handling models. This would avoid
the need for moves that slow down the non-trapping case.

To do so, we'd need to first emit the insn_start and then after translating the
given target-insn, update the insn_start parameters with the decoded insn
details.

I noticed that icount does a similar thing where it emits a movi and later
updates the immediate parameter with the real insn counter.

These patches illustrate a possible change by updating the icount code to
use a new tcg_set_insn_param() tcg call instead of directly peeking/poking
into tcg structures. This same mechanism can be used in the AArch64
translator.

Any thoughts on this approach? Or ideas on better options to achieve this?

Best regards,
Edgar

Edgar E. Iglesias (2):
  tcg: Add tcg_set_insn_param
  gen-icount: Use tcg_set_insn_param

 include/exec/gen-icount.h | 16 ++++++++--------
 tcg/tcg.h                 |  6 ++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

-- 
1.9.1

[Qemu-devel] [RFC 1/2] tcg: Add tcg_set_insn_param

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add tcg_set_insn_param as a mechanism to modify an insn
parameter after emiting the insn. This is useful for icount
and also for embedding fault information for a specific insn.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 tcg/tcg.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index a696922..9f2f4b8 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -585,6 +585,12 @@ struct TCGContext {
 
 extern TCGContext tcg_ctx;
 
+static inline void tcg_set_insn_param(int op_idx, int arg, TCGArg v)
+{
+    int op_argi = tcg_ctx.gen_op_buf[op_idx].args;
+    tcg_ctx.gen_opparam_buf[op_argi + arg] = v;
+}
+
 /* The number of opcodes emitted so far.  */
 static inline int tcg_op_buf_count(void)
 {
-- 
1.9.1

[Qemu-devel] [RFC 2/2] gen-icount: Use tcg_set_insn_param

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Use tcg_set_insn_param() instead of directly accessing internal
tcg data structures to update an insn param.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 include/exec/gen-icount.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 05d89d3..a011324 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -5,14 +5,13 @@
 
 /* Helpers for instruction counting code generation.  */
 
-static TCGArg *icount_arg;
+static int icount_start_insn_idx;
 static TCGLabel *icount_label;
 static TCGLabel *exitreq_label;
 
 static inline void gen_tb_start(TranslationBlock *tb)
 {
     TCGv_i32 count, flag, imm;
-    int i;
 
     exitreq_label = gen_new_label();
     flag = tcg_temp_new_i32();
@@ -31,13 +30,12 @@ static inline void gen_tb_start(TranslationBlock *tb)
                    -ENV_OFFSET + offsetof(CPUState, icount_decr.u32));
 
     imm = tcg_temp_new_i32();
+    /* We emit a movi with a dummy immediate argument. Keep the insn index
+     * of the movi so that we later (when we know the actual insn count)
+     * can update the immediate argument with the actual insn count.  */
+    icount_start_insn_idx = tcg_op_buf_count();
     tcg_gen_movi_i32(imm, 0xdeadbeef);
 
-    /* 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,9 @@ 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;
+        /* Update the num_insn immediate parameter now that we know
+         * the actual insn count.  */
+        tcg_set_insn_param(icount_start_insn_idx, 1, num_insns);
         gen_set_label(icount_label);
         tcg_gen_exit_tb((uintptr_t)tb + TB_EXIT_ICOUNT_EXPIRED);
     }
-- 
1.9.1

Re: [Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Richard Henderson]

On 01/08/2016 08:25 AM, Edgar E. Iglesias wrote:
> To do so, we'd need to first emit the insn_start and then after translating the
> given target-insn, update the insn_start parameters with the decoded insn
> details.

Fair enough.

> Any thoughts on this approach? Or ideas on better options to achieve this?

This looks like a nice cleanup already.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Lluís Vilanova]

Edgar E Iglesias writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> Hi,

> On AArch64, when some load/stores trap under specific conditions, a set of
> detailed info describing the insn is provided to the trap handler (e.g size
> of the access, target registers, insn-length mode etc).
> This specific info is known at translation time and Peter suggested that
> we have a look at the insn_start mechanism to see if we can reuse it
> to pass along the info to the exception handling models. This would avoid
> the need for moves that slow down the non-trapping case.

> To do so, we'd need to first emit the insn_start and then after translating the
> given target-insn, update the insn_start parameters with the decoded insn
> details.

> I noticed that icount does a similar thing where it emits a movi and later
> updates the immediate parameter with the real insn counter.

> These patches illustrate a possible change by updating the icount code to
> use a new tcg_set_insn_param() tcg call instead of directly peeking/poking
> into tcg structures. This same mechanism can be used in the AArch64
> translator.

> Any thoughts on this approach? Or ideas on better options to achieve this?

Great! I implemented a similar thing long time ago. In my case the machinery is
completely hidden under the concept of "value promises" in TCG (i.e., the user
does not need to know about TCG internals like tcg_op_buf_count):

   // create promise
   TCGv_promise_i32 imm_p;
   TCGv_i32 imm = tcg_const_promise_i32(&imm_p); // akin to tcg_const_i32()
   ...
   // operate with promised immediate 'imm'
   ...
   // resolve promised value
   tcg_set_promise_i32(imm_p, resolved_value);

Here's the changes (the diff on a single page returns a 500 error, so use the
per-file diff links):

https://projects.gso.ac.upc.edu/projects/qemu-dbi/repository/revisions/c85708f0845e4b983ebd6d6977cf3186e7bedba6

Note that this was written a long time ago, so it would probably need adapting.


Cheers,
  Lluis

Re: [Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Peter Maydell]

On 11 January 2016 at 20:16, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Great! I implemented a similar thing long time ago. In my case the machinery is
> completely hidden under the concept of "value promises" in TCG (i.e., the user
> does not need to know about TCG internals like tcg_op_buf_count):
>
>    // create promise
>    TCGv_promise_i32 imm_p;
>    TCGv_i32 imm = tcg_const_promise_i32(&imm_p); // akin to tcg_const_i32()
>    ...
>    // operate with promised immediate 'imm'
>    ...
>    // resolve promised value
>    tcg_set_promise_i32(imm_p, resolved_value);

I think this is definitely a nicer API if we're going to
have more than a very few uses -- gen_icount kind of gets
away with looking under the hood of the tcg data structures
because it's a sort of internal thing itself, but wider
use would definitely benefit from a more formal API.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Lluís Vilanova]

Peter Maydell writes:

> On 11 January 2016 at 20:16, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Great! I implemented a similar thing long time ago. In my case the machinery is
>> completely hidden under the concept of "value promises" in TCG (i.e., the user
>> does not need to know about TCG internals like tcg_op_buf_count):
>> 
>> // create promise
>> TCGv_promise_i32 imm_p;
>> TCGv_i32 imm = tcg_const_promise_i32(&imm_p); // akin to tcg_const_i32()
>> ...
>> // operate with promised immediate 'imm'
>> ...
>> // resolve promised value
>> tcg_set_promise_i32(imm_p, resolved_value);

> I think this is definitely a nicer API if we're going to
> have more than a very few uses -- gen_icount kind of gets
> away with looking under the hood of the tcg data structures
> because it's a sort of internal thing itself, but wider
> use would definitely benefit from a more formal API.

If this is not time-critical, I can prepare an updated patch by Friday, or let
Edgar do it. Both are fine by me.


Cheers,
  Lluis

Re: [Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Edgar E. Iglesias]

On Tue, Jan 12, 2016 at 01:12:29PM +0100, Lluís Vilanova wrote:
> Peter Maydell writes:
> 
> > On 11 January 2016 at 20:16, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> >> Great! I implemented a similar thing long time ago. In my case the machinery is
> >> completely hidden under the concept of "value promises" in TCG (i.e., the user
> >> does not need to know about TCG internals like tcg_op_buf_count):
> >> 
> >> // create promise
> >> TCGv_promise_i32 imm_p;
> >> TCGv_i32 imm = tcg_const_promise_i32(&imm_p); // akin to tcg_const_i32()
> >> ...
> >> // operate with promised immediate 'imm'
> >> ...
> >> // resolve promised value
> >> tcg_set_promise_i32(imm_p, resolved_value);
> 
> > I think this is definitely a nicer API if we're going to
> > have more than a very few uses -- gen_icount kind of gets
> > away with looking under the hood of the tcg data structures
> > because it's a sort of internal thing itself, but wider
> > use would definitely benefit from a more formal API.
> 
> If this is not time-critical, I can prepare an updated patch by Friday, or let
> Edgar do it. Both are fine by me.

It would be awesome if you could rebase your series.

Thanks Lluis

Cheers,
Edgar

Re: [Qemu-devel] [RFC 0/2] tcg-icount: Add and use tcg_set_insn_param to update tcg insn params

[Lluís Vilanova]

Edgar E Iglesias writes:

> On Tue, Jan 12, 2016 at 01:12:29PM +0100, Lluís Vilanova wrote:
>> Peter Maydell writes:
>> 
>> > On 11 January 2016 at 20:16, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> >> Great! I implemented a similar thing long time ago. In my case the machinery is
>> >> completely hidden under the concept of "value promises" in TCG (i.e., the user
>> >> does not need to know about TCG internals like tcg_op_buf_count):
>> >> 
>> >> // create promise
>> >> TCGv_promise_i32 imm_p;
>> >> TCGv_i32 imm = tcg_const_promise_i32(&imm_p); // akin to tcg_const_i32()
>> >> ...
>> >> // operate with promised immediate 'imm'
>> >> ...
>> >> // resolve promised value
>> >> tcg_set_promise_i32(imm_p, resolved_value);
>> 
>> > I think this is definitely a nicer API if we're going to
>> > have more than a very few uses -- gen_icount kind of gets
>> > away with looking under the hood of the tcg data structures
>> > because it's a sort of internal thing itself, but wider
>> > use would definitely benefit from a more formal API.
>> 
>> If this is not time-critical, I can prepare an updated patch by Friday, or let
>> Edgar do it. Both are fine by me.

> It would be awesome if you could rebase your series.

Ok, then I'll rebase it on Friday.

Cheers,
  Lluis