Functions which modify virtual machine state (such as virtual
registers stored in memory) must not be marked TCG_CALL_NO_WG as that
tells the optimizer that virtual registers values already loaded in
machine registers are still valid, hence discards any changes which
these helpers may have made.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/rx/helper.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/rx/helper.h b/target/rx/helper.h
index ebb4739474..ac21f4fddd 100644
--- a/target/rx/helper.h
+++ b/target/rx/helper.h
@@ -13,18 +13,18 @@ DEF_HELPER_FLAGS_2(ftoi, TCG_CALL_NO_WG, i32, env, f32)
 DEF_HELPER_FLAGS_2(round, TCG_CALL_NO_WG, i32, env, f32)
 DEF_HELPER_FLAGS_2(itof, TCG_CALL_NO_WG, f32, env, i32)
 DEF_HELPER_2(set_fpsw, void, env, i32)
-DEF_HELPER_FLAGS_2(racw, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_psw_rte, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_psw, TCG_CALL_NO_WG, void, env, i32)
+DEF_HELPER_2(racw, void, env, i32)
+DEF_HELPER_2(set_psw_rte, void, env, i32)
+DEF_HELPER_2(set_psw, void, env, i32)
 DEF_HELPER_1(pack_psw, i32, env)
 DEF_HELPER_FLAGS_3(div, TCG_CALL_NO_WG, i32, env, i32, i32)
 DEF_HELPER_FLAGS_3(divu, TCG_CALL_NO_WG, i32, env, i32, i32)
-DEF_HELPER_FLAGS_1(scmpu, TCG_CALL_NO_WG, void, env)
+DEF_HELPER_1(scmpu, void, env)
 DEF_HELPER_1(smovu, void, env)
 DEF_HELPER_1(smovf, void, env)
 DEF_HELPER_1(smovb, void, env)
 DEF_HELPER_2(sstr, void, env, i32)
-DEF_HELPER_FLAGS_2(swhile, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(suntil, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(rmpa, TCG_CALL_NO_WG, void, env, i32)
+DEF_HELPER_2(swhile, void, env, i32)
+DEF_HELPER_2(suntil, void, env, i32)
+DEF_HELPER_2(rmpa, void, env, i32)
 DEF_HELPER_1(satr, void, env)
-- 
2.47.2

> Functions which modify virtual machine state (such as virtual
> registers stored in memory) must not be marked TCG_CALL_NO_WG as that
> tells the optimizer that virtual registers values already loaded in
> machine registers are still valid, hence discards any changes which
> these helpers may have made.

I still don't understand the restrictions on using these flags. I just
had to disable this flag for other helpers which only set conditions
codes in PSW and FPSW. Is that expected? When are these flags supposed
to be valid?

-- 
-keith

On 2/15/25 01:21, Keith Packard via wrote:
> 
>> Functions which modify virtual machine state (such as virtual
>> registers stored in memory) must not be marked TCG_CALL_NO_WG as that
>> tells the optimizer that virtual registers values already loaded in
>> machine registers are still valid, hence discards any changes which
>> these helpers may have made.
> 
> I still don't understand the restrictions on using these flags. I just
> had to disable this flag for other helpers which only set conditions
> codes in PSW and FPSW. Is that expected? When are these flags supposed
> to be valid?

Yes, that's expected.

The state of affairs is not helped by the rx target's misuse of tcg globals.

A target should define tcg globals for values that are used frequently for emulation.  The 
bits of the PSW certainly fit that bill, because they're touched by most arithmetic 
operations.

However, fpsw, bpsw, bpc, isp, fintv, and intb are only used in move_to/from_cr and RTFI. 
This is infrequent, so simply loading and storing to env is preferred.  E.g.

     tcg_gen_ld_i32(value, tcg_env, offsetof(CPURXState, fpsw));


r~

On 2/15/25 09:09, Richard Henderson wrote:
> On 2/15/25 01:21, Keith Packard via wrote:
>>
>>> Functions which modify virtual machine state (such as virtual
>>> registers stored in memory) must not be marked TCG_CALL_NO_WG as that
>>> tells the optimizer that virtual registers values already loaded in
>>> machine registers are still valid, hence discards any changes which
>>> these helpers may have made.
>>
>> I still don't understand the restrictions on using these flags. I just
>> had to disable this flag for other helpers which only set conditions
>> codes in PSW and FPSW. Is that expected? When are these flags supposed
>> to be valid?
> 
> Yes, that's expected.
> 
> The state of affairs is not helped by the rx target's misuse of tcg globals.
> 
> A target should define tcg globals for values that are used frequently for emulation.  The 
> bits of the PSW certainly fit that bill, because they're touched by most arithmetic 
> operations.
> 
> However, fpsw, bpsw, bpc, isp, fintv, and intb are only used in move_to/from_cr and RTFI. 
> This is infrequent, so simply loading and storing to env is preferred.  E.g.
> 
>      tcg_gen_ld_i32(value, tcg_env, offsetof(CPURXState, fpsw));
> 

To finish my thought here,

> +DEF_HELPER_2(set_psw_rte, void, env, i32)
> +DEF_HELPER_2(set_psw, void, env, i32)
> +DEF_HELPER_1(scmpu, void, env)
> +DEF_HELPER_2(swhile, void, env, i32)
> +DEF_HELPER_2(suntil, void, env, i32)
> +DEF_HELPER_2(rmpa, void, env, i32)

are absolutely correct, in that they modify all of the psw_* globals.

> +DEF_HELPER_2(racw, void, env, i32)

is currently correct, in that it modifies cpu_acc.  That said, there's no reason that 
function could not be expanded inline:

     tcg_gen_shli_i64(cpu_acc, cpu_acc, imm);
     tcg_gen_addi_i64(cpu_acc, cpu_acc, 0x0000000080000000ull);
     tcg_gen_smin_i64(cpu_acc, cpu_acc, tcg_constant_i64(0x00007fff00000000ull));
     tcg_gen_smax_i64(cpu_acc, cpu_acc, tcg_constant_i64(0xffff800000000000ull));
     tcg_gen_andi_i64(cpu_acc, cpu_acc, 0xffffffff00000000ull);


r~