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