| Message ID | 20210818164321.2474534-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/mips: Replace TARGET_WORDS_BIGENDIAN by cpu_is_bigendian() | expand |
On 8/18/21 6:43 AM, Philippe Mathieu-Daudé wrote: > The target endianess information is stored in the BigEndian > bit of the Config0 register in CP0. > > As a first step, replace the GET_OFFSET() macro by an inlined > get_offset() function, passing CPUMIPSState as argument. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/mips/tcg/ldst_helper.c | 57 +++++++++++++++++++++-------------- > 1 file changed, 35 insertions(+), 22 deletions(-) > > diff --git a/target/mips/tcg/ldst_helper.c b/target/mips/tcg/ldst_helper.c > index d42812b8a6a..97e7ad7d7a4 100644 > --- a/target/mips/tcg/ldst_helper.c > +++ b/target/mips/tcg/ldst_helper.c > @@ -52,31 +52,44 @@ HELPER_LD_ATOMIC(lld, ldq, 0x7, (target_ulong)) > > #endif /* !CONFIG_USER_ONLY */ > > +static inline bool cpu_is_bigendian(CPUMIPSState *env) > +{ > + return extract32(env->CP0_Config0, CP0C0_BE, 1); > +} > + > #ifdef TARGET_WORDS_BIGENDIAN > #define GET_LMASK(v) ((v) & 3) > -#define GET_OFFSET(addr, offset) (addr + (offset)) > #else > #define GET_LMASK(v) (((v) & 3) ^ 3) > -#define GET_OFFSET(addr, offset) (addr - (offset)) > #endif > > +static inline target_ulong get_offset(CPUMIPSState *env, > + target_ulong addr, int offset) > +{ > + if (cpu_is_bigendian(env)) { > + return addr + offset; > + } else { > + return addr - offset; > + } > +} > + > void helper_swl(CPUMIPSState *env, target_ulong arg1, target_ulong arg2, > int mem_idx) > { > cpu_stb_mmuidx_ra(env, arg2, (uint8_t)(arg1 >> 24), mem_idx, GETPC()); > > if (GET_LMASK(arg2) <= 2) { > - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 1), (uint8_t)(arg1 >> 16), > + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 1), (uint8_t)(arg1 >> 16), > mem_idx, GETPC()); > } > > if (GET_LMASK(arg2) <= 1) { > - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 2), (uint8_t)(arg1 >> 8), > + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 2), (uint8_t)(arg1 >> 8), > mem_idx, GETPC()); So... yes, this is an improvement, but it's now substituting a constant for a runtime variable many times over. I think you should drop get_offset() entirely and replace it with int dir = cpu_is_bigendian(env) ? 1 : -1; stb(env, arg2 + 1 * dir, data); stb(env, arg2 + 2 * dir, data); Alternately, bite the bullet and split the function(s) into two, explicitly endian versions: helper_swl_be, helper_swl_le, etc. r~
On 8/18/21 6:56 PM, Richard Henderson wrote: > On 8/18/21 6:43 AM, Philippe Mathieu-Daudé wrote: >> The target endianess information is stored in the BigEndian >> bit of the Config0 register in CP0. >> >> As a first step, replace the GET_OFFSET() macro by an inlined >> get_offset() function, passing CPUMIPSState as argument. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> target/mips/tcg/ldst_helper.c | 57 +++++++++++++++++++++-------------- >> 1 file changed, 35 insertions(+), 22 deletions(-) >> >> diff --git a/target/mips/tcg/ldst_helper.c >> b/target/mips/tcg/ldst_helper.c >> index d42812b8a6a..97e7ad7d7a4 100644 >> --- a/target/mips/tcg/ldst_helper.c >> +++ b/target/mips/tcg/ldst_helper.c >> @@ -52,31 +52,44 @@ HELPER_LD_ATOMIC(lld, ldq, 0x7, (target_ulong)) >> #endif /* !CONFIG_USER_ONLY */ >> +static inline bool cpu_is_bigendian(CPUMIPSState *env) >> +{ >> + return extract32(env->CP0_Config0, CP0C0_BE, 1); >> +} >> + >> #ifdef TARGET_WORDS_BIGENDIAN >> #define GET_LMASK(v) ((v) & 3) >> -#define GET_OFFSET(addr, offset) (addr + (offset)) >> #else >> #define GET_LMASK(v) (((v) & 3) ^ 3) >> -#define GET_OFFSET(addr, offset) (addr - (offset)) >> #endif >> +static inline target_ulong get_offset(CPUMIPSState *env, >> + target_ulong addr, int offset) >> +{ >> + if (cpu_is_bigendian(env)) { >> + return addr + offset; >> + } else { >> + return addr - offset; >> + } >> +} >> + >> void helper_swl(CPUMIPSState *env, target_ulong arg1, target_ulong >> arg2, >> int mem_idx) >> { >> cpu_stb_mmuidx_ra(env, arg2, (uint8_t)(arg1 >> 24), mem_idx, >> GETPC()); >> if (GET_LMASK(arg2) <= 2) { >> - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 1), (uint8_t)(arg1 >> >> 16), >> + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 1), >> (uint8_t)(arg1 >> 16), >> mem_idx, GETPC()); >> } >> if (GET_LMASK(arg2) <= 1) { >> - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 2), (uint8_t)(arg1 >> >> 8), >> + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 2), >> (uint8_t)(arg1 >> 8), >> mem_idx, GETPC()); > > So... yes, this is an improvement, but it's now substituting a constant > for a runtime variable many times over. Oops indeed. > I think you should drop > get_offset() entirely and replace it with > > int dir = cpu_is_bigendian(env) ? 1 : -1; > > stb(env, arg2 + 1 * dir, data); > > stb(env, arg2 + 2 * dir, data); > > Alternately, bite the bullet and split the function(s) into two, > explicitly endian versions: helper_swl_be, helper_swl_le, etc. I'll go for the easier path ;)
On 8/18/21 11:31 AM, Philippe Mathieu-Daudé wrote: >> I think you should drop >> get_offset() entirely and replace it with >> >> int dir = cpu_is_bigendian(env) ? 1 : -1; >> >> stb(env, arg2 + 1 * dir, data); >> >> stb(env, arg2 + 2 * dir, data); >> >> Alternately, bite the bullet and split the function(s) into two, >> explicitly endian versions: helper_swl_be, helper_swl_le, etc. > > I'll go for the easier path ;) It's not really more difficult. static inline void do_swl(env, uint32_t val, target_ulong addr, int midx, int dir, unsigned lmask, uintptr_t ra) { cpu_stb_mmuidx_ra(env, addr, val >> 24, midx, ra); if (lmask <= 2) { cpu_stb_mmuidx_ra(env, addr + 1 * dir, val >> 16, midx, ra); } if (lmask <= 1) { cpu_stb_mmuidx_ra(env, addr + 1 * dir, val >> 8, midx, ra); } if (lmask == 0) { cpu_stb_mmuidx_ra(env, addr + 1 * dir, val, midx, ra); } } void helper_swl_be(env, val, addr, midx) { do_swl(env, val, addr, midx, 1, addr & 3, GETPC()); } void helper_swl_le(env, val, addr, midx) { do_swl(env, val, addr, midx, -1, ~addr & 3, GETPC()); } Although I do wonder if this is strictly correct with respect to atomicity. In my tcg/mips unaligned patch set, I assumed that lwl+lwr of an aligned address produces two atomic 32-bit loads, which result in a complete atomic load at the end. Should we be doing something like void helper_swl_be(env, val, addr, midx) { uintptr_t ra = GETPC(); switch (addr & 3) { case 0: cpu_stl_be_mmuidx_ra(env, val, addr, midx, ra); break; case 1: cpu_stb_mmuidx_ra(env, val >> 24, addr, midx, ra); cpu_stw_be_mmuidx_ra(env, val >> 16, addr + 1, midx, ra); break; case 2: cpu_stw_be_mmuidx_ra(env, val >> 16, addr, midx, ra); break; case 3: cpu_stb_mmuidx_ra(env, val >> 24, addr, midx, ra); break; } } void helper_swl_le(env, val, addr, midx) { uintptr_t ra = GETPC(); /* * We want to use stw and stl for atomicity, but want any * fault to report ADDR, not the aligned address. */ probe_write(env, addr, 0, midx, ra); switch (addr & 3) { case 3: cpu_stl_le_mmuidx_ra(env, val, addr - 3, midx, ra); break; case 1: cpu_stw_le_mmuidx_ra(env, val >> 16, addr - 1, midx, ra); break; case 2: cpu_stw_le_mmuidx_ra(env, val >> 8, addr - 2, midx, ra); /* fall through */ case 0: cpu_stb_mmuidx_ra(env, val >> 24, addr, midx, ra); break; } } etc. r~
diff --git a/target/mips/tcg/ldst_helper.c b/target/mips/tcg/ldst_helper.c index d42812b8a6a..97e7ad7d7a4 100644 --- a/target/mips/tcg/ldst_helper.c +++ b/target/mips/tcg/ldst_helper.c @@ -52,31 +52,44 @@ HELPER_LD_ATOMIC(lld, ldq, 0x7, (target_ulong)) #endif /* !CONFIG_USER_ONLY */ +static inline bool cpu_is_bigendian(CPUMIPSState *env) +{ + return extract32(env->CP0_Config0, CP0C0_BE, 1); +} + #ifdef TARGET_WORDS_BIGENDIAN #define GET_LMASK(v) ((v) & 3) -#define GET_OFFSET(addr, offset) (addr + (offset)) #else #define GET_LMASK(v) (((v) & 3) ^ 3) -#define GET_OFFSET(addr, offset) (addr - (offset)) #endif +static inline target_ulong get_offset(CPUMIPSState *env, + target_ulong addr, int offset) +{ + if (cpu_is_bigendian(env)) { + return addr + offset; + } else { + return addr - offset; + } +} + void helper_swl(CPUMIPSState *env, target_ulong arg1, target_ulong arg2, int mem_idx) { cpu_stb_mmuidx_ra(env, arg2, (uint8_t)(arg1 >> 24), mem_idx, GETPC()); if (GET_LMASK(arg2) <= 2) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 1), (uint8_t)(arg1 >> 16), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 1), (uint8_t)(arg1 >> 16), mem_idx, GETPC()); } if (GET_LMASK(arg2) <= 1) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 2), (uint8_t)(arg1 >> 8), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 2), (uint8_t)(arg1 >> 8), mem_idx, GETPC()); } if (GET_LMASK(arg2) == 0) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 3), (uint8_t)arg1, + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 3), (uint8_t)arg1, mem_idx, GETPC()); } } @@ -87,17 +100,17 @@ void helper_swr(CPUMIPSState *env, target_ulong arg1, target_ulong arg2, cpu_stb_mmuidx_ra(env, arg2, (uint8_t)arg1, mem_idx, GETPC()); if (GET_LMASK(arg2) >= 1) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -1), (uint8_t)(arg1 >> 8), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -1), (uint8_t)(arg1 >> 8), mem_idx, GETPC()); } if (GET_LMASK(arg2) >= 2) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -2), (uint8_t)(arg1 >> 16), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -2), (uint8_t)(arg1 >> 16), mem_idx, GETPC()); } if (GET_LMASK(arg2) == 3) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -3), (uint8_t)(arg1 >> 24), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -3), (uint8_t)(arg1 >> 24), mem_idx, GETPC()); } } @@ -119,37 +132,37 @@ void helper_sdl(CPUMIPSState *env, target_ulong arg1, target_ulong arg2, cpu_stb_mmuidx_ra(env, arg2, (uint8_t)(arg1 >> 56), mem_idx, GETPC()); if (GET_LMASK64(arg2) <= 6) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 1), (uint8_t)(arg1 >> 48), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 1), (uint8_t)(arg1 >> 48), mem_idx, GETPC()); } if (GET_LMASK64(arg2) <= 5) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 2), (uint8_t)(arg1 >> 40), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 2), (uint8_t)(arg1 >> 40), mem_idx, GETPC()); } if (GET_LMASK64(arg2) <= 4) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 3), (uint8_t)(arg1 >> 32), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 3), (uint8_t)(arg1 >> 32), mem_idx, GETPC()); } if (GET_LMASK64(arg2) <= 3) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 4), (uint8_t)(arg1 >> 24), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 4), (uint8_t)(arg1 >> 24), mem_idx, GETPC()); } if (GET_LMASK64(arg2) <= 2) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 5), (uint8_t)(arg1 >> 16), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 5), (uint8_t)(arg1 >> 16), mem_idx, GETPC()); } if (GET_LMASK64(arg2) <= 1) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 6), (uint8_t)(arg1 >> 8), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 6), (uint8_t)(arg1 >> 8), mem_idx, GETPC()); } if (GET_LMASK64(arg2) <= 0) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, 7), (uint8_t)arg1, + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, 7), (uint8_t)arg1, mem_idx, GETPC()); } } @@ -160,37 +173,37 @@ void helper_sdr(CPUMIPSState *env, target_ulong arg1, target_ulong arg2, cpu_stb_mmuidx_ra(env, arg2, (uint8_t)arg1, mem_idx, GETPC()); if (GET_LMASK64(arg2) >= 1) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -1), (uint8_t)(arg1 >> 8), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -1), (uint8_t)(arg1 >> 8), mem_idx, GETPC()); } if (GET_LMASK64(arg2) >= 2) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -2), (uint8_t)(arg1 >> 16), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -2), (uint8_t)(arg1 >> 16), mem_idx, GETPC()); } if (GET_LMASK64(arg2) >= 3) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -3), (uint8_t)(arg1 >> 24), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -3), (uint8_t)(arg1 >> 24), mem_idx, GETPC()); } if (GET_LMASK64(arg2) >= 4) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -4), (uint8_t)(arg1 >> 32), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -4), (uint8_t)(arg1 >> 32), mem_idx, GETPC()); } if (GET_LMASK64(arg2) >= 5) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -5), (uint8_t)(arg1 >> 40), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -5), (uint8_t)(arg1 >> 40), mem_idx, GETPC()); } if (GET_LMASK64(arg2) >= 6) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -6), (uint8_t)(arg1 >> 48), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -6), (uint8_t)(arg1 >> 48), mem_idx, GETPC()); } if (GET_LMASK64(arg2) == 7) { - cpu_stb_mmuidx_ra(env, GET_OFFSET(arg2, -7), (uint8_t)(arg1 >> 56), + cpu_stb_mmuidx_ra(env, get_offset(env, arg2, -7), (uint8_t)(arg1 >> 56), mem_idx, GETPC()); } }
The target endianess information is stored in the BigEndian bit of the Config0 register in CP0. As a first step, replace the GET_OFFSET() macro by an inlined get_offset() function, passing CPUMIPSState as argument. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- target/mips/tcg/ldst_helper.c | 57 +++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 22 deletions(-)