| Message ID | 20240524232121.284515-4-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Convert a64 advsimd to decodetree (part 1) | expand |
On Sat, 25 May 2024 at 00:25, Richard Henderson <richard.henderson@linaro.org> wrote: > > For all, rm == 15 is invalid. > Prior to v8, thumb with rm == 13 is invalid. > For PLDW, rn == 15 is invalid. > Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu > compared to a neoverse-n1 host. These are UNPREDICTABLE cases, not invalid. In general we don't try to match a specific implementation's UNPREDICTABLE choices. I think we're better off avoiding the mismatch by improving the risu patterns to avoid the UNPREDICTABLE cases. thanks -- PMM
On 5/28/24 06:18, Peter Maydell wrote: > On Sat, 25 May 2024 at 00:25, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> For all, rm == 15 is invalid. >> Prior to v8, thumb with rm == 13 is invalid. >> For PLDW, rn == 15 is invalid. > >> Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu >> compared to a neoverse-n1 host. > > These are UNPREDICTABLE cases, not invalid. In general > we don't try to match a specific implementation's > UNPREDICTABLE choices. > > I think we're better off avoiding the mismatch by improving > the risu patterns to avoid the UNPREDICTABLE cases. We do plenty of other treatments of UNPREDICTABLE as UNDEF (e.g. STREX). Why is this case any different? r~
On Tue, 28 May 2024 at 18:36, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 5/28/24 06:18, Peter Maydell wrote: > > On Sat, 25 May 2024 at 00:25, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> For all, rm == 15 is invalid. > >> Prior to v8, thumb with rm == 13 is invalid. > >> For PLDW, rn == 15 is invalid. > > > >> Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu > >> compared to a neoverse-n1 host. > > > > These are UNPREDICTABLE cases, not invalid. In general > > we don't try to match a specific implementation's > > UNPREDICTABLE choices. > > > > I think we're better off avoiding the mismatch by improving > > the risu patterns to avoid the UNPREDICTABLE cases. > > We do plenty of other treatments of UNPREDICTABLE as UNDEF (e.g. STREX). Why is this case > any different? It just seems like a lot of effort to go to. Sometimes we UNDEF for UNPREDICTABLEs, but quite often we say "the behaviour we get for free is fine, so no need to write extra code". In this particular case, also: * we'd need to go back and cross-check against older architecture manuals and also look at whether M-profile and A-profile are doing the same thing here * the "v8 loosens the UNPREDICTABLE case for m = 13 T1 encoding" looks suspiciously to me like a "nobody ever actually made this do anything except behave like the A1 encoding, so make T1 and A1 the same" kind of relaxation Basically, it would take me probably 15+ minutes to review the changes against the various versions of the docs and I don't think it's worth the effort :-) thanks -- PMM
On 5/29/24 06:32, Peter Maydell wrote: >> We do plenty of other treatments of UNPREDICTABLE as UNDEF (e.g. STREX). Why is this case >> any different? > > It just seems like a lot of effort to go to. Sometimes we > UNDEF for UNPREDICTABLEs, but quite often we say "the > behaviour we get for free is fine, so no need to write > extra code". > > In this particular case, also: > * we'd need to go back and cross-check against older > architecture manuals and also look at whether M-profile > and A-profile are doing the same thing here > * the "v8 loosens the UNPREDICTABLE case for m = 13 T1 > encoding" looks suspiciously to me like a "nobody ever > actually made this do anything except behave like the A1 > encoding, so make T1 and A1 the same" kind of relaxation > > Basically, it would take me probably 15+ minutes to review > the changes against the various versions of the docs and > I don't think it's worth the effort :-) Fair enough. I've sent a patch to update thumb.risu to avoid the problematic patterns. r~
diff --git a/target/arm/tcg/a32-uncond.decode b/target/arm/tcg/a32-uncond.decode index 2339de2e94..e1b1780d37 100644 --- a/target/arm/tcg/a32-uncond.decode +++ b/target/arm/tcg/a32-uncond.decode @@ -24,7 +24,9 @@ &empty !extern &i !extern imm +&r !extern rm &setend E +&nm rn rm # Branch with Link and Exchange @@ -61,9 +63,9 @@ PLD 1111 0101 -101 ---- 1111 ---- ---- ---- # (imm, lit) 5te PLDW 1111 0101 -001 ---- 1111 ---- ---- ---- # (imm, lit) 7mp PLI 1111 0100 -101 ---- 1111 ---- ---- ---- # (imm, lit) 7 -PLD 1111 0111 -101 ---- 1111 ----- -- 0 ---- # (register) 5te -PLDW 1111 0111 -001 ---- 1111 ----- -- 0 ---- # (register) 7mp -PLI 1111 0110 -101 ---- 1111 ----- -- 0 ---- # (register) 7 +PLD_rr 1111 0111 -101 ---- 1111 ----- -- 0 rm:4 &r +PLDW_rr 1111 0111 -001 rn:4 1111 ----- -- 0 rm:4 &nm +PLI_rr 1111 0110 -101 ---- 1111 ----- -- 0 rm:4 &r # Unallocated memory hints # diff --git a/target/arm/tcg/t32.decode b/target/arm/tcg/t32.decode index d327178829..1ec12442a4 100644 --- a/target/arm/tcg/t32.decode +++ b/target/arm/tcg/t32.decode @@ -28,6 +28,7 @@ &rrr_rot !extern rd rn rm rot &rrr !extern rd rn rm &rr !extern rd rm +&nm !extern rn rm &ri !extern rd imm &r !extern rm &i !extern imm @@ -472,7 +473,7 @@ STR_ri 1111 1000 1100 .... .... ............ @ldst_ri_pos } LDRBT_ri 1111 1000 0001 .... .... 1110 ........ @ldst_ri_unp { - PLD 1111 1000 0001 ---- 1111 000000 -- ---- # (register) + PLD_rr 1111 1000 0001 ---- 1111 000000 -- rm:4 &r LDRB_rr 1111 1000 0001 .... .... 000000 .. .... @ldst_rr } } @@ -492,7 +493,7 @@ STR_ri 1111 1000 1100 .... .... ............ @ldst_ri_pos } LDRHT_ri 1111 1000 0011 .... .... 1110 ........ @ldst_ri_unp { - PLDW 1111 1000 0011 ---- 1111 000000 -- ---- # (register) + PLDW_rr 1111 1000 0011 rn:4 1111 000000 -- rm:4 &nm LDRH_rr 1111 1000 0011 .... .... 000000 .. .... @ldst_rr } } @@ -520,7 +521,7 @@ STR_ri 1111 1000 1100 .... .... ............ @ldst_ri_pos } LDRSBT_ri 1111 1001 0001 .... .... 1110 ........ @ldst_ri_unp { - PLI 1111 1001 0001 ---- 1111 000000 -- ---- # (register) + PLI_rr 1111 1001 0001 ---- 1111 000000 -- rm:4 &r LDRSB_rr 1111 1001 0001 .... .... 000000 .. .... @ldst_rr } } diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c index 187eacffd9..7c09153b6e 100644 --- a/target/arm/tcg/translate.c +++ b/target/arm/tcg/translate.c @@ -8775,6 +8775,63 @@ static bool trans_PLI(DisasContext *s, arg_PLI *a) return ENABLE_ARCH_7; } +static bool prefetch_check_m(DisasContext *s, int rm) +{ + switch (rm) { + case 13: + /* SP allowed in v8 or with A1 encoding; rejected with T1. */ + return ENABLE_ARCH_8 || !s->thumb; + case 15: + /* PC always rejected. */ + return false; + default: + return true; + } +} + +static bool trans_PLD_rr(DisasContext *s, arg_PLD_rr *a) +{ + if (!ENABLE_ARCH_5TE) { + return false; + } + /* We cannot return false, because that leads to LDRB for thumb. */ + if (!prefetch_check_m(s, a->rm)) { + unallocated_encoding(s); + } + return true; +} + +static bool trans_PLDW_rr(DisasContext *s, arg_PLDW_rr *a) +{ + if (!arm_dc_feature(s, ARM_FEATURE_V7MP)) { + return false; + } + /* + * For A1, rn == 15 is UNPREDICTABLE. + * For T1, rn == 15 is PLD (literal). + */ + if (a->rn == 15) { + return false; + } + /* We cannot return false, because that leads to LDRH for thumb. */ + if (!prefetch_check_m(s, a->rm)) { + unallocated_encoding(s); + } + return true; +} + +static bool trans_PLI_rr(DisasContext *s, arg_PLI_rr *a) +{ + if (!ENABLE_ARCH_7) { + return false; + } + /* We cannot return false, because that leads to LDRSB for thumb. */ + if (!prefetch_check_m(s, a->rm)) { + unallocated_encoding(s); + } + return true; +} + /* * If-then */
For all, rm == 15 is invalid. Prior to v8, thumb with rm == 13 is invalid. For PLDW, rn == 15 is invalid. Fixes a RISU mismatch for the HINTSPACE pattern in t32.risu compared to a neoverse-n1 host. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/tcg/a32-uncond.decode | 8 +++-- target/arm/tcg/t32.decode | 7 ++-- target/arm/tcg/translate.c | 57 ++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 6 deletions(-)