Re: [Qemu-devel] [PATCH v6 08/11] target-avr: adding instruction translation

[Richard Henderson]

On 06/13/2016 01:25 PM, Michael Rolnik wrote:

what is the difference between tcg_gen_qemu_st16 and tcg_gen_qemu_st_tl


st16 is a legacy interface; st_tl is newer and has an argument that is a mask 
of MO_* bits.  MO_BEUW is a convenience name for MO_BE | MO_16.



r~

Re: [Qemu-devel] [PATCH v6 08/11] target-avr: adding instruction translation

[Michael Rolnik]

what is the difference between tcg_gen_qemu_st16 and tcg_gen_qemu_st_tl

On Mon, Jun 13, 2016 at 7:06 PM, Richard Henderson  wrote:

> On 06/12/2016 12:01 PM, Michael Rolnik wrote:
>
>> +void gen_push_ret(CPUAVRState *env, int ret)
>> +{
>> +if (avr_feature(env, AVR_FEATURE_1_BYTE_PC)) {
>> +
>> +TCGv t0 = tcg_const_i32((ret & 0xff));
>> +
>> +tcg_gen_qemu_st8(t0, cpu_sp, MMU_DATA_IDX);
>> +tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
>> +
>> +tcg_temp_free_i32(t0);
>> +} else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
>> +
>> +TCGv t0 = tcg_const_i32((ret & 0x00));
>> +
>> +tcg_gen_qemu_st16(t0, cpu_sp, MMU_DATA_IDX);
>> +tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
>>
>
> This stores to the wrong bytes.  You need
>
> tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
> tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
>
> +} else if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
>> +
>> +TCGv t0 = tcg_const_i32((ret & 0xff));
>> +TCGv t1 = tcg_const_i32((ret & 0x00) >> 8);
>> +
>> +tcg_gen_qemu_st8(t0, cpu_sp, MMU_DATA_IDX);
>> +tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
>> +
>> +tcg_gen_qemu_st16(t1, cpu_sp, MMU_DATA_IDX);
>> +tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
>>
>
> Similarly.
>
> +void gen_pop_ret(CPUAVRState *env, TCGv ret)
>> +{
>> +if (avr_feature(env, AVR_FEATURE_1_BYTE_PC)) {
>> +
>> +tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
>> +tcg_gen_qemu_ld8u(ret, cpu_sp, MMU_DATA_IDX);
>> +} else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
>> +
>> +tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
>> +tcg_gen_qemu_ld16u(ret, cpu_sp, MMU_DATA_IDX);
>>
>
> Similarly,
>
> tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
>
>
> +} else if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
>> +
>> +TCGv t0 = tcg_temp_new_i32();
>> +
>> +tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
>> +tcg_gen_qemu_ld8u(ret, cpu_sp, MMU_DATA_IDX);
>> +
>> +tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
>> +tcg_gen_qemu_ld16u(t0, cpu_sp, MMU_DATA_IDX);
>> +
>> +tcg_gen_shli_tl(t0, t0, 16);
>> +tcg_gen_or_tl(ret, ret, t0);
>>
>
> You're putting t0 at the wrong end.
>
> tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> tcg_gen_qemu_ld_tl(t0, cpu_sp, MMU_DATA_IDX, MO_UB);
> tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> tcg_gen_deposit_tl(ret, ret, t0, 16, 8);
>
>
>
> r~
>



-- 
Best Regards,
Michael Rolnik

Re: [Qemu-devel] [PATCH v6 08/11] target-avr: adding instruction translation

[Richard Henderson]

On 06/12/2016 12:01 PM, Michael Rolnik wrote:

+void gen_push_ret(CPUAVRState *env, int ret)
+{
+if (avr_feature(env, AVR_FEATURE_1_BYTE_PC)) {
+
+TCGv t0 = tcg_const_i32((ret & 0xff));
+
+tcg_gen_qemu_st8(t0, cpu_sp, MMU_DATA_IDX);
+tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
+
+tcg_temp_free_i32(t0);
+} else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
+
+TCGv t0 = tcg_const_i32((ret & 0x00));
+
+tcg_gen_qemu_st16(t0, cpu_sp, MMU_DATA_IDX);
+tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);


This stores to the wrong bytes.  You need

tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_BEUW);
tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);


+} else if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
+
+TCGv t0 = tcg_const_i32((ret & 0xff));
+TCGv t1 = tcg_const_i32((ret & 0x00) >> 8);
+
+tcg_gen_qemu_st8(t0, cpu_sp, MMU_DATA_IDX);
+tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
+
+tcg_gen_qemu_st16(t1, cpu_sp, MMU_DATA_IDX);
+tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);


Similarly.


+void gen_pop_ret(CPUAVRState *env, TCGv ret)
+{
+if (avr_feature(env, AVR_FEATURE_1_BYTE_PC)) {
+
+tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
+tcg_gen_qemu_ld8u(ret, cpu_sp, MMU_DATA_IDX);
+} else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
+
+tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
+tcg_gen_qemu_ld16u(ret, cpu_sp, MMU_DATA_IDX);


Similarly,

tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_BEUW);
tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);



+} else if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
+
+TCGv t0 = tcg_temp_new_i32();
+
+tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
+tcg_gen_qemu_ld8u(ret, cpu_sp, MMU_DATA_IDX);
+
+tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
+tcg_gen_qemu_ld16u(t0, cpu_sp, MMU_DATA_IDX);
+
+tcg_gen_shli_tl(t0, t0, 16);
+tcg_gen_or_tl(ret, ret, t0);


You're putting t0 at the wrong end.

tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
tcg_gen_qemu_ld_tl(t0, cpu_sp, MMU_DATA_IDX, MO_UB);
tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_BEUW); 
tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
tcg_gen_deposit_tl(ret, ret, t0, 16, 8);



r~