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