| Message ID | 145287210479.25408.17499099590726683703.stgit@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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~
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
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~
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
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
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
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); /**
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(+)