| Message ID | 20220601125355.1266165-1-matheus.ferst@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: fix vbpermd in big endian hosts | expand |
On 01/06/2022 15:21, Philippe Mathieu-Daudé via wrote: > +Mark for commit ef96e3ae96. > > On 1/6/22 14:53, matheus.ferst@eldorado.org.br wrote: >> From: Matheus Ferst <matheus.ferst@eldorado.org.br> >> >> The extract64 arguments are not endian dependent as they are only used >> for bitwise operations. The current behavior in little-endian hosts is >> correct; since the indexes in VRB are in PowerISA-ordering, we should >> always invert the value before calling extract64. Also, using the VsrD >> macro, we can have a single EXTRACT_BIT definition for big and >> little-endian with the correct behavior. >> >> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> >> --- >> Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The >> complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX, >> but it will need more testing. For now, we're just changing what is >> necessary to fix the instruction. >> --- >> target/ppc/int_helper.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c >> index 105b626d1b..4c5d3f03f8 100644 >> --- a/target/ppc/int_helper.c >> +++ b/target/ppc/int_helper.c >> @@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8) >> #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)]) >> #define VBPERMD_INDEX(i) (i) >> #define VBPERMQ_DW(index) (((index) & 0x40) != 0) >> -#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1)) >> #else >> #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)]) >> #define VBPERMD_INDEX(i) (1 - i) >> #define VBPERMQ_DW(index) (((index) & 0x40) == 0) >> -#define EXTRACT_BIT(avr, i, index) \ >> - (extract64((avr)->u64[1 - i], 63 - index, 1)) >> #endif >> +#define EXTRACT_BIT(avr, i, index) \ >> + (extract64((avr)->VsrD(i), 63 - index, 1)) >> void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) >> { I'm not too familiar with vbpermd, however in general the use of the VsrX() macros is the right way to ensure things work correctly on both big-endian and little-endian hosts, so it looks fine to me. FWIW with all the great improvements being done in this area, I think that Matheus and Daniel have picked things up really quickly and have a much better test setup than the G4 Mac Mini I used to do the original gvec work. If I happen to spot something on the mailing list then I'll likely reply, but otherwise I'm happy to allow things to progress without requiring an explicit Ack from me (these days my testing is mostly confined to checking that MacOS 9/X boot okay). ATB, Mark.
On 6/1/22 05:53, matheus.ferst@eldorado.org.br wrote: > From: Matheus Ferst<matheus.ferst@eldorado.org.br> > > The extract64 arguments are not endian dependent as they are only used > for bitwise operations. The current behavior in little-endian hosts is > correct; since the indexes in VRB are in PowerISA-ordering, we should > always invert the value before calling extract64. Also, using the VsrD > macro, we can have a single EXTRACT_BIT definition for big and > little-endian with the correct behavior. > > Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br> > --- > Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The > complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX, > but it will need more testing. For now, we're just changing what is > necessary to fix the instruction. > --- > target/ppc/int_helper.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel On 6/1/22 09:53, matheus.ferst@eldorado.org.br wrote: > From: Matheus Ferst <matheus.ferst@eldorado.org.br> > > The extract64 arguments are not endian dependent as they are only used > for bitwise operations. The current behavior in little-endian hosts is > correct; since the indexes in VRB are in PowerISA-ordering, we should > always invert the value before calling extract64. Also, using the VsrD > macro, we can have a single EXTRACT_BIT definition for big and > little-endian with the correct behavior. > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> > --- > Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The > complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX, > but it will need more testing. For now, we're just changing what is > necessary to fix the instruction. > --- > target/ppc/int_helper.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c > index 105b626d1b..4c5d3f03f8 100644 > --- a/target/ppc/int_helper.c > +++ b/target/ppc/int_helper.c > @@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8) > #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)]) > #define VBPERMD_INDEX(i) (i) > #define VBPERMQ_DW(index) (((index) & 0x40) != 0) > -#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1)) > #else > #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)]) > #define VBPERMD_INDEX(i) (1 - i) > #define VBPERMQ_DW(index) (((index) & 0x40) == 0) > -#define EXTRACT_BIT(avr, i, index) \ > - (extract64((avr)->u64[1 - i], 63 - index, 1)) > #endif > +#define EXTRACT_BIT(avr, i, index) \ > + (extract64((avr)->VsrD(i), 63 - index, 1)) > > void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) > {
diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c index 105b626d1b..4c5d3f03f8 100644 --- a/target/ppc/int_helper.c +++ b/target/ppc/int_helper.c @@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8) #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)]) #define VBPERMD_INDEX(i) (i) #define VBPERMQ_DW(index) (((index) & 0x40) != 0) -#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1)) #else #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)]) #define VBPERMD_INDEX(i) (1 - i) #define VBPERMQ_DW(index) (((index) & 0x40) == 0) -#define EXTRACT_BIT(avr, i, index) \ - (extract64((avr)->u64[1 - i], 63 - index, 1)) #endif +#define EXTRACT_BIT(avr, i, index) \ + (extract64((avr)->VsrD(i), 63 - index, 1)) void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b) {