| Message ID | 20211021194547.672988-17-matheus.ferst@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PowerISA v3.1 instruction batch | expand |
On 10/21/21 12:45 PM, matheus.ferst@eldorado.org.br wrote: > +static bool do_vins_VX_uim4(DisasContext *ctx, arg_VX_uim4 *a, int size, > + void (*gen_helper)(TCGv_ptr, TCGv_ptr, TCGv_i64, TCGv)) > +{ > + REQUIRE_INSNS_FLAGS2(ctx, ISA310); > + REQUIRE_VECTOR(ctx); > + > + if (a->uim > (16 - size)) { > + qemu_log_mask(LOG_GUEST_ERROR, "Invalid index for VINS* at" > + " 0x" TARGET_FMT_lx ", UIM = %d > %d\n", ctx->cia, a->uim, > + 16 - size); > + return true; > + } Does this really do nothing on real hw? I know the manual says undefined, but I would have expected SIGILL. > +#if defined(TARGET_PPC64) > + return do_vinsx(ctx, a->vrt, size, false, tcg_constant_tl(a->uim), > + cpu_gpr[a->vrb], gen_helper); > +#else > + bool ok; > + TCGv_i64 val; > + > + val = tcg_temp_new_i64(); > + tcg_gen_extu_tl_i64(val, cpu_gpr[a->vrb]); > + > + ok = do_vinsx(ctx, a->vrt, size, false, tcg_constant_tl(a->uim), val, > + gen_helper); > + > + tcg_temp_free_i64(val); > + return ok; > +#endif Similarly wrt target_ulong. r~
On 23/10/2021 01:42, Richard Henderson wrote: > [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você > possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de > e-mail suspeito entre imediatamente em contato com o DTI. > > On 10/21/21 12:45 PM, matheus.ferst@eldorado.org.br wrote: >> +static bool do_vins_VX_uim4(DisasContext *ctx, arg_VX_uim4 *a, int size, >> + void (*gen_helper)(TCGv_ptr, TCGv_ptr, >> TCGv_i64, TCGv)) >> +{ >> + REQUIRE_INSNS_FLAGS2(ctx, ISA310); >> + REQUIRE_VECTOR(ctx); >> + >> + if (a->uim > (16 - size)) { >> + qemu_log_mask(LOG_GUEST_ERROR, "Invalid index for VINS* at" >> + " 0x" TARGET_FMT_lx ", UIM = %d > %d\n", ctx->cia, a->uim, >> + 16 - size); >> + return true; >> + } > > Does this really do nothing on real hw? We don't have access to the real hardware yet, so our reference is the POWER10 Functional Simulator (Mambo). Maybe someone from IBM can run a test for us, but Mambo usually does the right thing, especially in "simple mode." > I know the manual says undefined, but I would have expected SIGILL. It says that "if UIM is greater than N, the result is undefined." My first read was also that the outcome is "boundedly undefined," but I guess it can be understood as "the resulting value in VRT will be undefined" (like when the pseudo-code uses "VRT <- 0xUUUU_..._UUUU"), in which case this patch and Mambo are correct.
On 10/26/21 7:33 AM, Matheus K. Ferst wrote: > It says that "if UIM is greater than N, the result is undefined." My first read was also > that the outcome is "boundedly undefined," but I guess it can be understood as "the > resulting value in VRT will be undefined" (like when the pseudo-code uses "VRT <- > 0xUUUU_..._UUUU"), in which case this patch and Mambo are correct. If the reference simulator is fine with it, I am too. I'm just a bit disappointed with the laxness of the pseudocode -- they've got that 0xuuuu syntax elsewhere, but not here. r~
On Tue, Oct 26, 2021 at 09:58:15AM -0700, Richard Henderson wrote: > On 10/26/21 7:33 AM, Matheus K. Ferst wrote: > > It says that "if UIM is greater than N, the result is undefined." My > > first read was also that the outcome is "boundedly undefined," but I > > guess it can be understood as "the resulting value in VRT will be > > undefined" (like when the pseudo-code uses "VRT <- 0xUUUU_..._UUUU"), in > > which case this patch and Mambo are correct. > > If the reference simulator is fine with it, I am too. FYI, it appears that the hardware does a partial insert, per an experiment: ``` 1: x/i $pc => 0x100006d4 <foo+4>: vinsw v2,r3,14 (gdb) p $v2.v4_int32 $1 = {0x1, 0x1, 0x1, 0x1} (gdb) p $r3 $2 = 0x12345678 (gdb) nexti (gdb) p $v2.v4_int32 $3 = {0x1234, 0x1, 0x1, 0x1} ```` > I'm just a bit disappointed with the laxness of the pseudocode -- they've > got that 0xuuuu syntax elsewhere, but not here. PC
On 26/10/2021 15:45, Paul A. Clarke wrote: > On Tue, Oct 26, 2021 at 09:58:15AM -0700, Richard Henderson wrote: >> On 10/26/21 7:33 AM, Matheus K. Ferst wrote: >>> It says that "if UIM is greater than N, the result is undefined." My >>> first read was also that the outcome is "boundedly undefined," but I >>> guess it can be understood as "the resulting value in VRT will be >>> undefined" (like when the pseudo-code uses "VRT <- 0xUUUU_..._UUUU"), in >>> which case this patch and Mambo are correct. >> >> If the reference simulator is fine with it, I am too. > > FYI, it appears that the hardware does a partial insert, per an experiment: > ``` > 1: x/i $pc > => 0x100006d4 <foo+4>: vinsw v2,r3,14 > (gdb) p $v2.v4_int32 > $1 = {0x1, 0x1, 0x1, 0x1} > (gdb) p $r3 > $2 = 0x12345678 > (gdb) nexti > (gdb) p $v2.v4_int32 > $3 = {0x1234, 0x1, 0x1, 0x1} > ```` Thanks for this test Paul. I'll add a comment about the hardware behavior.
diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode index b794424496..e1f76aac34 100644 --- a/target/ppc/insn32.decode +++ b/target/ppc/insn32.decode @@ -44,6 +44,9 @@ &VX vrt vra vrb @VX ...... vrt:5 vra:5 vrb:5 .......... . &VX +&VX_uim4 vrt uim vrb +@VX_uim4 ...... vrt:5 . uim:4 vrb:5 ........... &VX_uim4 + &X rt ra rb @X ...... rt:5 ra:5 rb:5 .......... . &X @@ -353,5 +356,8 @@ VINSWRX 000100 ..... ..... ..... 01110001111 @VX VINSDLX 000100 ..... ..... ..... 01011001111 @VX VINSDRX 000100 ..... ..... ..... 01111001111 @VX +VINSW 000100 ..... - .... ..... 00011001111 @VX_uim4 +VINSD 000100 ..... - .... ..... 00111001111 @VX_uim4 + VSLDBI 000100 ..... ..... ..... 00 ... 010110 @VN VSRDBI 000100 ..... ..... ..... 01 ... 010110 @VN diff --git a/target/ppc/translate/vmx-impl.c.inc b/target/ppc/translate/vmx-impl.c.inc index 0c5f0dcf32..3b526977e4 100644 --- a/target/ppc/translate/vmx-impl.c.inc +++ b/target/ppc/translate/vmx-impl.c.inc @@ -1283,6 +1283,37 @@ static bool do_vinsx_VX(DisasContext *ctx, arg_VX *a, int size, bool right, #endif } +static bool do_vins_VX_uim4(DisasContext *ctx, arg_VX_uim4 *a, int size, + void (*gen_helper)(TCGv_ptr, TCGv_ptr, TCGv_i64, TCGv)) +{ + REQUIRE_INSNS_FLAGS2(ctx, ISA310); + REQUIRE_VECTOR(ctx); + + if (a->uim > (16 - size)) { + qemu_log_mask(LOG_GUEST_ERROR, "Invalid index for VINS* at" + " 0x" TARGET_FMT_lx ", UIM = %d > %d\n", ctx->cia, a->uim, + 16 - size); + return true; + } + +#if defined(TARGET_PPC64) + return do_vinsx(ctx, a->vrt, size, false, tcg_constant_tl(a->uim), + cpu_gpr[a->vrb], gen_helper); +#else + bool ok; + TCGv_i64 val; + + val = tcg_temp_new_i64(); + tcg_gen_extu_tl_i64(val, cpu_gpr[a->vrb]); + + ok = do_vinsx(ctx, a->vrt, size, false, tcg_constant_tl(a->uim), val, + gen_helper); + + tcg_temp_free_i64(val); + return ok; +#endif +} + TRANS(VINSBLX, do_vinsx_VX, 1, false, gen_helper_VINSBLX) TRANS(VINSHLX, do_vinsx_VX, 2, false, gen_helper_VINSHLX) TRANS(VINSWLX, do_vinsx_VX, 4, false, gen_helper_VINSWLX) @@ -1293,6 +1324,9 @@ TRANS(VINSHRX, do_vinsx_VX, 2, true, gen_helper_VINSHLX) TRANS(VINSWRX, do_vinsx_VX, 4, true, gen_helper_VINSWLX) TRANS(VINSDRX, do_vinsx_VX, 8, true, gen_helper_VINSDLX) +TRANS(VINSW, do_vins_VX_uim4, 4, gen_helper_VINSWLX) +TRANS(VINSD, do_vins_VX_uim4, 8, gen_helper_VINSDLX) + static void gen_vsldoi(DisasContext *ctx) { TCGv_ptr ra, rb, rd;