| Message ID | 20200625170018.64265-9-ljp@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add several Power ISA 3.1 32/64-bit vector instructions | expand |
On 6/25/20 10:00 AM, Lijun Pan wrote: > +#define VDIV_MOD_DO(name, op, element, sign, bit) \ > + void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) \ > + { \ > + int i; \ > + \ > + \ > + for (i = 0; i < ARRAY_SIZE(r->element); i++) { \ > + if (unlikely((b->element[i] == 0) || \ > + (sign && \ > + (b->element[i] == UINT##bit##_MAX) && \ > + (a->element[i] == INT##bit##_MIN)))) \ > + continue; \ > + r->element[i] = a->element[i] op b->element[i]; \ > + } \ > + } Missing braces for the if. Extra blank line before the for. I see that the ISA document says divide-by-zero produces an undefined result, so leaving the previous contents does seem to be within the letter of the law. However... Are you able to test what real hardware produces? It would be nice (but not required) to match if it is simple to do so. Whichever way we go with the undefined result, this deserves a comment. r~
> On Jun 25, 2020, at 1:37 PM, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 6/25/20 10:00 AM, Lijun Pan wrote: >> +#define VDIV_MOD_DO(name, op, element, sign, bit) \ >> + void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) \ >> + { \ >> + int i; \ >> + \ >> + \ >> + for (i = 0; i < ARRAY_SIZE(r->element); i++) { \ >> + if (unlikely((b->element[i] == 0) || \ >> + (sign && \ >> + (b->element[i] == UINT##bit##_MAX) && \ >> + (a->element[i] == INT##bit##_MIN)))) \ >> + continue; \ >> + r->element[i] = a->element[i] op b->element[i]; \ >> + } \ >> + } > > Missing braces for the if. Extra blank line before the for. No, the braces are enough. "unlikely" is to describe the whole logic, eg. if (unlikely( (divisor == 0) || (sign && (divisor == 0xFFFFFFFF) && (dividend = 0x80000000) ) ) ) I will remove that blank line. > > I see that the ISA document says divide-by-zero produces an undefined result, > so leaving the previous contents does seem to be within the letter of the law. > > However... Are you able to test what real hardware produces? It would be nice > (but not required) to match if it is simple to do so. > > Whichever way we go with the undefined result, this deserves a comment. I will add “continue; / * Undefined, No Special Registers Altered */ " Lijun
On 6/25/20 2:15 PM, Lijun Pan wrote: > > >> On Jun 25, 2020, at 1:37 PM, Richard Henderson <richard.henderson@linaro.org> wrote: >> >> On 6/25/20 10:00 AM, Lijun Pan wrote: >>> +#define VDIV_MOD_DO(name, op, element, sign, bit) \ >>> + void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) \ >>> + { \ >>> + int i; \ >>> + \ >>> + \ >>> + for (i = 0; i < ARRAY_SIZE(r->element); i++) { \ >>> + if (unlikely((b->element[i] == 0) || \ >>> + (sign && \ >>> + (b->element[i] == UINT##bit##_MAX) && \ >>> + (a->element[i] == INT##bit##_MIN)))) \ >>> + continue; \ >>> + r->element[i] = a->element[i] op b->element[i]; \ >>> + } \ >>> + } >> >> Missing braces for the if. Extra blank line before the for. > > No, the braces are enough. No they are not. See CODING_STYLE.rst. r~
On Thu, Jun 25, 2020 at 08:53:54PM -0700, Richard Henderson wrote: > On 6/25/20 2:15 PM, Lijun Pan wrote: > > > > > >> On Jun 25, 2020, at 1:37 PM, Richard Henderson <richard.henderson@linaro.org> wrote: > >> > >> On 6/25/20 10:00 AM, Lijun Pan wrote: > >>> +#define VDIV_MOD_DO(name, op, element, sign, bit) \ > >>> + void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) \ > >>> + { \ > >>> + int i; \ > >>> + \ > >>> + \ > >>> + for (i = 0; i < ARRAY_SIZE(r->element); i++) { \ > >>> + if (unlikely((b->element[i] == 0) || \ > >>> + (sign && \ > >>> + (b->element[i] == UINT##bit##_MAX) && \ > >>> + (a->element[i] == INT##bit##_MIN)))) \ > >>> + continue; \ > >>> + r->element[i] = a->element[i] op b->element[i]; \ > >>> + } \ > >>> + } > >> > >> Missing braces for the if. Extra blank line before the for. > > > > No, the braces are enough. > > No they are not. See CODING_STYLE.rst. I suspect there is some confusion in terms here. Lijun, what Richard is saying is that the qemu coding style requires braces { } around if blocks, even if they're a single statement. Your response seemed to be discussing the brackets ( ) which are fine.
diff --git a/target/ppc/helper.h b/target/ppc/helper.h index 0036788919..70a14029ca 100644 --- a/target/ppc/helper.h +++ b/target/ppc/helper.h @@ -188,6 +188,14 @@ DEF_HELPER_3(vmulhsw, void, avr, avr, avr) DEF_HELPER_3(vmulhuw, void, avr, avr, avr) DEF_HELPER_3(vmulhsd, void, avr, avr, avr) DEF_HELPER_3(vmulhud, void, avr, avr, avr) +DEF_HELPER_3(vdivsw, void, avr, avr, avr) +DEF_HELPER_3(vdivuw, void, avr, avr, avr) +DEF_HELPER_3(vdivsd, void, avr, avr, avr) +DEF_HELPER_3(vdivud, void, avr, avr, avr) +DEF_HELPER_3(vmodsw, void, avr, avr, avr) +DEF_HELPER_3(vmoduw, void, avr, avr, avr) +DEF_HELPER_3(vmodsd, void, avr, avr, avr) +DEF_HELPER_3(vmodud, void, avr, avr, avr) DEF_HELPER_3(vslo, void, avr, avr, avr) DEF_HELPER_3(vsro, void, avr, avr, avr) DEF_HELPER_3(vsrv, void, avr, avr, avr) diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c index 57d6767f60..283f5a00af 100644 --- a/target/ppc/int_helper.c +++ b/target/ppc/int_helper.c @@ -1121,6 +1121,32 @@ void helper_vmulhud(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) mulu64(&discard, &r->u64[1], a->u64[1], b->u64[1]); } +#define VDIV_MOD_DO(name, op, element, sign, bit) \ + void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) \ + { \ + int i; \ + \ + \ + for (i = 0; i < ARRAY_SIZE(r->element); i++) { \ + if (unlikely((b->element[i] == 0) || \ + (sign && \ + (b->element[i] == UINT##bit##_MAX) && \ + (a->element[i] == INT##bit##_MIN)))) \ + continue; \ + r->element[i] = a->element[i] op b->element[i]; \ + } \ + } +VDIV_MOD_DO(divsw, /, s32, 1, 32) +VDIV_MOD_DO(divuw, /, u32, 0, 32) +VDIV_MOD_DO(divsd, /, s64, 1, 64) +VDIV_MOD_DO(divud, /, u64, 0, 64) +VDIV_MOD_DO(modsw, %, s32, 1, 32) +VDIV_MOD_DO(moduw, %, u32, 0, 32) +VDIV_MOD_DO(modsd, %, s64, 1, 64) +VDIV_MOD_DO(modud, %, u64, 0, 64) +#undef VDIV_MOD_DO + + void helper_vperm(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, ppc_avr_t *c) { diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 6634b38f3a..d4ea7eccfd 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -388,6 +388,9 @@ GEN_OPCODE3(name, opc1, opc2, opc3, opc4, inval, type, type2) #define GEN_HANDLER2_E_2(name, onam, opc1, opc2, opc3, opc4, inval, typ, typ2) \ GEN_OPCODE4(name, onam, opc1, opc2, opc3, opc4, inval, typ, typ2) +#define GEN_HANDLER_BOTH(name, opc1, opc2, opc3, inval0, inval1, type0, type1) \ +GEN_OPCODE_DUAL(name, opc1, opc2, opc3, inval0, inval1, type0, type1) + typedef struct opcode_t { unsigned char opc1, opc2, opc3, opc4; #if HOST_LONG_BITS == 64 /* Explicitly align to 64 bits */ diff --git a/target/ppc/translate/vmx-impl.inc.c b/target/ppc/translate/vmx-impl.inc.c index 0910807232..ac5e820541 100644 --- a/target/ppc/translate/vmx-impl.inc.c +++ b/target/ppc/translate/vmx-impl.inc.c @@ -798,6 +798,9 @@ static void trans_vclzd(DisasContext *ctx) tcg_temp_free_i64(avr); } +static void gen_vexptefp(DisasContext *ctx); +static void gen_vlogefp(DisasContext *ctx); + GEN_VXFORM(vmuloub, 4, 0); GEN_VXFORM(vmulouh, 4, 1); GEN_VXFORM(vmulouw, 4, 2); @@ -822,6 +825,18 @@ GEN_VXFORM(vmulhsw, 4, 14); GEN_VXFORM_DUAL(vmulesw, PPC_ALTIVEC, PPC_NONE, vmulhsw, PPC_NONE, PPC2_ISA310); GEN_VXFORM(vmulhsd, 4, 15); +GEN_VXFORM(vdivuw, 5, 2); +GEN_VXFORM(vdivud, 5, 3); +GEN_VXFORM(vdivsw, 5, 6); +GEN_VXFORM_DUAL_EXT(vexptefp, PPC_ALTIVEC, PPC_NONE, 0x001f0000, + vdivsw, PPC_NONE, PPC2_ISA310, 0x00000000); +GEN_VXFORM(vdivsd, 5, 7); +GEN_VXFORM_DUAL_EXT(vlogefp, PPC_ALTIVEC, PPC_NONE, 0x001f0000, + vdivsd, PPC_NONE, PPC2_ISA310, 0x00000000); +GEN_VXFORM(vmoduw, 5, 26); +GEN_VXFORM(vmodud, 5, 27); +GEN_VXFORM(vmodsw, 5, 30); +GEN_VXFORM(vmodsd, 5, 31); GEN_VXFORM_V(vslb, MO_8, tcg_gen_gvec_shlv, 2, 4); GEN_VXFORM_V(vslh, MO_16, tcg_gen_gvec_shlv, 2, 5); GEN_VXFORM_V(vslw, MO_32, tcg_gen_gvec_shlv, 2, 6); diff --git a/target/ppc/translate/vmx-ops.inc.c b/target/ppc/translate/vmx-ops.inc.c index f3f4855111..528458cb25 100644 --- a/target/ppc/translate/vmx-ops.inc.c +++ b/target/ppc/translate/vmx-ops.inc.c @@ -54,6 +54,11 @@ GEN_HANDLER_E(name, 0x04, opc2, opc3, 0x00000000, PPC_NONE, PPC2_ISA310) #define GEN_VXFORM_DUAL(name0, name1, opc2, opc3, type0, type1) \ GEN_HANDLER_E(name0##_##name1, 0x4, opc2, opc3, 0x00000000, type0, type1) +#define GEN_VXFORM_DUAL_BOTH(name0, name1, opc2, opc3, inval0, \ + inval1, type0, type1) \ +GEN_HANDLER_BOTH(name0##_##name1, 0x4, opc2, opc3, inval0, \ + inval1, type0, type1) + #define GEN_VXRFORM_DUAL(name0, name1, opc2, opc3, tp0, tp1) \ GEN_HANDLER_E(name0##_##name1, 0x4, opc2, opc3, 0x00000000, tp0, tp1), \ GEN_HANDLER_E(name0##_##name1, 0x4, opc2, (opc3 | 0x10), 0x00000000, tp0, tp1), @@ -116,6 +121,16 @@ GEN_VXFORM(vmulesb, 4, 12), GEN_VXFORM(vmulesh, 4, 13), GEN_VXFORM_DUAL(vmulesw, vmulhsw, 4, 14, PPC_ALTIVEC, PPC_NONE), GEN_VXFORM_310(vmulhsd, 4, 15), +GEN_VXFORM_310(vdivuw, 5, 2), +GEN_VXFORM_310(vdivud, 5, 3), +GEN_VXFORM_DUAL_BOTH(vexptefp, vdivsw, 5, 6, 0x001f0000, 0x00000000, + PPC_ALTIVEC, PPC2_ISA310), +GEN_VXFORM_DUAL_BOTH(vlogefp, vdivsd, 5, 7, 0x001f0000, 0x00000000, + PPC_ALTIVEC, PPC2_ISA310), +GEN_VXFORM_310(vmoduw, 5, 26), +GEN_VXFORM_310(vmodud, 5, 27), +GEN_VXFORM_310(vmodsw, 5, 30), +GEN_VXFORM_310(vmodsd, 5, 31), GEN_VXFORM(vslb, 2, 4), GEN_VXFORM(vslh, 2, 5), GEN_VXFORM_DUAL(vslw, vrlwnm, 2, 6, PPC_ALTIVEC, PPC_NONE), @@ -259,8 +274,6 @@ GEN_VXFORM_NOA(vupkhpx, 7, 13), GEN_VXFORM_NOA(vupklpx, 7, 15), GEN_VXFORM_NOA(vrefp, 5, 4), GEN_VXFORM_NOA(vrsqrtefp, 5, 5), -GEN_VXFORM_NOA(vexptefp, 5, 6), -GEN_VXFORM_NOA(vlogefp, 5, 7), GEN_VXFORM_NOA(vrfim, 5, 11), GEN_VXFORM_NOA(vrfin, 5, 8), GEN_VXFORM_NOA(vrfip, 5, 10),
vdivsw: Vector Divide Signed Word vdivuw: Vector Divide Unsigned Word vdivsd: Vector Divide Signed Doubleword vdivud: Vector Divide Unsigned Doubleword vmodsw: Vector Modulo Signed Word vmoduw: Vector Modulo Unsigned Word vmodsd: Vector Modulo Signed Doubleword vmodud: Vector Modulo Unsigned Doubleword Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- v3: add missing divided-by-zero, divided-by-(-1) handling target/ppc/helper.h | 8 ++++++++ target/ppc/int_helper.c | 26 ++++++++++++++++++++++++++ target/ppc/translate.c | 3 +++ target/ppc/translate/vmx-impl.inc.c | 15 +++++++++++++++ target/ppc/translate/vmx-ops.inc.c | 17 +++++++++++++++-- 5 files changed, 67 insertions(+), 2 deletions(-)