Message ID | 1465561779-24588-1-git-send-email-james.greenhalgh@arm.com |
---|---|
State | New |
Headers | show |
On 10 June 2016 at 14:29, James Greenhalgh <james.greenhalgh@arm.com> wrote: > > Hi, > > My autotester picked up some issues with the vcvt{ds}_n_* intrinsics > added in r237200. > Hi, What tests does your autotester perform? I haven't noticed these problems when running the GCC testsuite on the usual aarch64 targets. I'm interested in increasing coverage, if doable. Thanks Christophe > The iterators in this pattern do not resolve, as they have not been > explicitly tied to the mode iterator (rather than the code iterator) > used by the pattern. > > This fixup adds the attribute tags, allowing the patterns to work > correctly. > > Additionally, the types assigned to these instructions were wrong, and > would permit the immediate operand to be in a register. This will then > develop in to an ICE as the patterns require an immediate operand, and so > won't match. The ICE can be exposed by writing a wrapping function around > the vcvtd_n_* intrinsics, which forces the immediate operand to a register. > We have the infrastructure to error to the user rather than ICEing, but it > needs some different types, which this patch adds. > > I've checked this with an aarch64-none-elf test run, and run it through > several rounds of my autotester for aarch64-none-elf and > aarch64_be-none-elf. > > OK? > > Thanks, > James > > --- > 2016-06-10 James Greenhalgh <james.greenhalgh@arm.com> > > * config/aarch64/aarch64.md > (<FCVT_F2FIXED:fcvt_fixed_insn><GPF:mode>3): Add attributes to > iterators. > (<FCVT_FIXED2F:fcvt_fixed_insn><GPI:mode>3): Likewise. Correct > attributes. > * config/aarch64/aarch64-builtins.c > (aarch64_types_binop_uss_qualifiers): Delete. > (TYPES_BINOP_USS): Likewise. > (aarch64_types_binop_sus_qualifiers): Likewise. > (TYPES_BINOP_SUS): Likewise. > (aarch64_types_fcvt_from_unsigned_qualifiers): New. > (TYPES_FCVTIMM_SUS): Likewise. > * config/aarch64/aarch64-simd-builtins.def (scvtf): Use SHIFTIMM > rather than BINOP. > (ucvtf): Use FCVTIMM_SUS rather than BINOP_SUS. > (fcvtzs): Use SHIFTIMM rather than BINOP. > (fcvtzu): Use SHIFTIMM_USS rather than BINOP_USS. >
On Fri, Jun 17, 2016 at 04:25:31PM +0200, Christophe Lyon wrote: > On 10 June 2016 at 14:29, James Greenhalgh <james.greenhalgh@arm.com> wrote: > > > > Hi, > > > > My autotester picked up some issues with the vcvt{ds}_n_* intrinsics > > added in r237200. > > > Hi, > > What tests does your autotester perform? I haven't noticed these > problems when running the GCC testsuite on the usual aarch64 > targets. I'm interested in increasing coverage, if doable. Hi Christophe, I think we've spoken about this before [1], but the autotester is using an internal testsuite that is not feasible to share upstream. To see the sorts of tests that we're running, have a look at the LLVM testsuite. If the layout of the testsuite hasn't changed since I last looked, you should be able to find an example at: <llvm-testsuite>/SingleSource/UnitTests/Vector/AArch64/aarch64_neon_intrinsics.c Hope that helps, James [1]: https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00775.html
On 17 June 2016 at 16:44, James Greenhalgh <james.greenhalgh@arm.com> wrote: > On Fri, Jun 17, 2016 at 04:25:31PM +0200, Christophe Lyon wrote: >> On 10 June 2016 at 14:29, James Greenhalgh <james.greenhalgh@arm.com> wrote: >> > >> > Hi, >> > >> > My autotester picked up some issues with the vcvt{ds}_n_* intrinsics >> > added in r237200. >> > >> Hi, >> >> What tests does your autotester perform? I haven't noticed these >> problems when running the GCC testsuite on the usual aarch64 >> targets. I'm interested in increasing coverage, if doable. > > Hi Christophe, > > I think we've spoken about this before [1], but the autotester is using > an internal testsuite that is not feasible to share upstream. Ha, indeed. I wasn't sure you were referring to the same tests. > > To see the sorts of tests that we're running, have a look at the LLVM > testsuite. If the layout of the testsuite hasn't changed since I last > looked, you should be able to find an example at: > > <llvm-testsuite>/SingleSource/UnitTests/Vector/AArch64/aarch64_neon_intrinsics.c > > Hope that helps, > James > > [1]: https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00775.html >
On Fri, Jun 10, 2016 at 01:29:39PM +0100, James Greenhalgh wrote: > > Hi, > > My autotester picked up some issues with the vcvt{ds}_n_* intrinsics > added in r237200. > > The iterators in this pattern do not resolve, as they have not been > explicitly tied to the mode iterator (rather than the code iterator) > used by the pattern. > > This fixup adds the attribute tags, allowing the patterns to work > correctly. > > Additionally, the types assigned to these instructions were wrong, and > would permit the immediate operand to be in a register. This will then > develop in to an ICE as the patterns require an immediate operand, and so > won't match. The ICE can be exposed by writing a wrapping function around > the vcvtd_n_* intrinsics, which forces the immediate operand to a register. > We have the infrastructure to error to the user rather than ICEing, but it > needs some different types, which this patch adds. > > I've checked this with an aarch64-none-elf test run, and run it through > several rounds of my autotester for aarch64-none-elf and > aarch64_be-none-elf. > > OK? *ping* https://gcc.gnu.org/ml/gcc-patches/2016-06/msg00805.html Thanks, James > --- > 2016-06-10 James Greenhalgh <james.greenhalgh@arm.com> > > * config/aarch64/aarch64.md > (<FCVT_F2FIXED:fcvt_fixed_insn><GPF:mode>3): Add attributes to > iterators. > (<FCVT_FIXED2F:fcvt_fixed_insn><GPI:mode>3): Likewise. Correct > attributes. > * config/aarch64/aarch64-builtins.c > (aarch64_types_binop_uss_qualifiers): Delete. > (TYPES_BINOP_USS): Likewise. > (aarch64_types_binop_sus_qualifiers): Likewise. > (TYPES_BINOP_SUS): Likewise. > (aarch64_types_fcvt_from_unsigned_qualifiers): New. > (TYPES_FCVTIMM_SUS): Likewise. > * config/aarch64/aarch64-simd-builtins.def (scvtf): Use SHIFTIMM > rather than BINOP. > (ucvtf): Use FCVTIMM_SUS rather than BINOP_SUS. > (fcvtzs): Use SHIFTIMM rather than BINOP. > (fcvtzu): Use SHIFTIMM_USS rather than BINOP_USS. >
On 10/06/16 13:29, James Greenhalgh wrote: > > Hi, > > My autotester picked up some issues with the vcvt{ds}_n_* intrinsics > added in r237200. > > The iterators in this pattern do not resolve, as they have not been > explicitly tied to the mode iterator (rather than the code iterator) > used by the pattern. > > This fixup adds the attribute tags, allowing the patterns to work > correctly. > > Additionally, the types assigned to these instructions were wrong, and > would permit the immediate operand to be in a register. This will then > develop in to an ICE as the patterns require an immediate operand, and so > won't match. The ICE can be exposed by writing a wrapping function around > the vcvtd_n_* intrinsics, which forces the immediate operand to a register. > We have the infrastructure to error to the user rather than ICEing, but it > needs some different types, which this patch adds. > > I've checked this with an aarch64-none-elf test run, and run it through > several rounds of my autotester for aarch64-none-elf and > aarch64_be-none-elf. > > OK? > OK. R. > Thanks, > James > > --- > 2016-06-10 James Greenhalgh <james.greenhalgh@arm.com> > > * config/aarch64/aarch64.md > (<FCVT_F2FIXED:fcvt_fixed_insn><GPF:mode>3): Add attributes to > iterators. > (<FCVT_FIXED2F:fcvt_fixed_insn><GPI:mode>3): Likewise. Correct > attributes. > * config/aarch64/aarch64-builtins.c > (aarch64_types_binop_uss_qualifiers): Delete. > (TYPES_BINOP_USS): Likewise. > (aarch64_types_binop_sus_qualifiers): Likewise. > (TYPES_BINOP_SUS): Likewise. > (aarch64_types_fcvt_from_unsigned_qualifiers): New. > (TYPES_FCVTIMM_SUS): Likewise. > * config/aarch64/aarch64-simd-builtins.def (scvtf): Use SHIFTIMM > rather than BINOP. > (ucvtf): Use FCVTIMM_SUS rather than BINOP_SUS. > (fcvtzs): Use SHIFTIMM rather than BINOP. > (fcvtzu): Use SHIFTIMM_USS rather than BINOP_USS. > > > 0001-Patch-AArch64-Fixup-to-fcvt-patterns-added-in-r23720.patch > > > diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c > index 262ea1c..6b90b2a 100644 > --- a/gcc/config/aarch64/aarch64-builtins.c > +++ b/gcc/config/aarch64/aarch64-builtins.c > @@ -139,14 +139,6 @@ aarch64_types_binop_ssu_qualifiers[SIMD_MAX_BUILTIN_ARGS] > = { qualifier_none, qualifier_none, qualifier_unsigned }; > #define TYPES_BINOP_SSU (aarch64_types_binop_ssu_qualifiers) > static enum aarch64_type_qualifiers > -aarch64_types_binop_uss_qualifiers[SIMD_MAX_BUILTIN_ARGS] > - = { qualifier_unsigned, qualifier_none, qualifier_none }; > -#define TYPES_BINOP_USS (aarch64_types_binop_uss_qualifiers) > -static enum aarch64_type_qualifiers > -aarch64_types_binop_sus_qualifiers[SIMD_MAX_BUILTIN_ARGS] > - = { qualifier_none, qualifier_unsigned, qualifier_none }; > -#define TYPES_BINOP_SUS (aarch64_types_binop_sus_qualifiers) > -static enum aarch64_type_qualifiers > aarch64_types_binopp_qualifiers[SIMD_MAX_BUILTIN_ARGS] > = { qualifier_poly, qualifier_poly, qualifier_poly }; > #define TYPES_BINOPP (aarch64_types_binopp_qualifiers) > @@ -181,6 +173,10 @@ aarch64_types_shift_to_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS] > = { qualifier_unsigned, qualifier_none, qualifier_immediate }; > #define TYPES_SHIFTIMM_USS (aarch64_types_shift_to_unsigned_qualifiers) > static enum aarch64_type_qualifiers > +aarch64_types_fcvt_from_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS] > + = { qualifier_none, qualifier_unsigned, qualifier_immediate }; > +#define TYPES_FCVTIMM_SUS (aarch64_types_fcvt_from_unsigned_qualifiers) > +static enum aarch64_type_qualifiers > aarch64_types_unsigned_shift_qualifiers[SIMD_MAX_BUILTIN_ARGS] > = { qualifier_unsigned, qualifier_unsigned, qualifier_immediate }; > #define TYPES_USHIFTIMM (aarch64_types_unsigned_shift_qualifiers) > diff --git a/gcc/config/aarch64/aarch64-simd-builtins.def b/gcc/config/aarch64/aarch64-simd-builtins.def > index 1332734..02d465b 100644 > --- a/gcc/config/aarch64/aarch64-simd-builtins.def > +++ b/gcc/config/aarch64/aarch64-simd-builtins.def > @@ -447,10 +447,10 @@ > BUILTIN_VSDQ_HSI (QUADOP_LANE, sqrdmlsh_laneq, 0) > > /* Implemented by <FCVT_F2FIXED/FIXED2F:fcvt_fixed_insn><*><*>3. */ > - BUILTIN_VSDQ_SDI (BINOP, scvtf, 3) > - BUILTIN_VSDQ_SDI (BINOP_SUS, ucvtf, 3) > - BUILTIN_VALLF (BINOP, fcvtzs, 3) > - BUILTIN_VALLF (BINOP_USS, fcvtzu, 3) > + BUILTIN_VSDQ_SDI (SHIFTIMM, scvtf, 3) > + BUILTIN_VSDQ_SDI (FCVTIMM_SUS, ucvtf, 3) > + BUILTIN_VALLF (SHIFTIMM, fcvtzs, 3) > + BUILTIN_VALLF (SHIFTIMM_USS, fcvtzu, 3) > > /* Implemented by aarch64_rsqrte<mode>. */ > BUILTIN_VALLF (UNOP, rsqrte, 0) > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 926f2da..b3ae42b 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -4640,8 +4640,8 @@ > FCVT_F2FIXED))] > "" > "@ > - <FCVT_F2FIXED:fcvt_fixed_insn>\t%<w1>0, %<s>1, #%2 > - <FCVT_F2FIXED:fcvt_fixed_insn>\t%<s>0, %<s>1, #%2" > + <FCVT_F2FIXED:fcvt_fixed_insn>\t%<GPF:w1>0, %<GPF:s>1, #%2 > + <FCVT_F2FIXED:fcvt_fixed_insn>\t%<GPF:s>0, %<GPF:s>1, #%2" > [(set_attr "type" "f_cvtf2i, neon_fp_to_int_<GPF:Vetype>") > (set_attr "fp" "yes, *") > (set_attr "simd" "*, yes")] > @@ -4654,8 +4654,8 @@ > FCVT_FIXED2F))] > "" > "@ > - <FCVT_FIXED2F:fcvt_fixed_insn>\t%<s>0, %<w1>1, #%2 > - <FCVT_FIXED2F:fcvt_fixed_insn>\t%<s>0, %<s>1, #%2" > + <FCVT_FIXED2F:fcvt_fixed_insn>\t%<GPI:v>0, %<GPI:w>1, #%2 > + <FCVT_FIXED2F:fcvt_fixed_insn>\t%<GPI:v>0, %<GPI:v>1, #%2" > [(set_attr "type" "f_cvti2f, neon_int_to_fp_<GPI:Vetype>") > (set_attr "fp" "yes, *") > (set_attr "simd" "*, yes")] >
diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c index 262ea1c..6b90b2a 100644 --- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -139,14 +139,6 @@ aarch64_types_binop_ssu_qualifiers[SIMD_MAX_BUILTIN_ARGS] = { qualifier_none, qualifier_none, qualifier_unsigned }; #define TYPES_BINOP_SSU (aarch64_types_binop_ssu_qualifiers) static enum aarch64_type_qualifiers -aarch64_types_binop_uss_qualifiers[SIMD_MAX_BUILTIN_ARGS] - = { qualifier_unsigned, qualifier_none, qualifier_none }; -#define TYPES_BINOP_USS (aarch64_types_binop_uss_qualifiers) -static enum aarch64_type_qualifiers -aarch64_types_binop_sus_qualifiers[SIMD_MAX_BUILTIN_ARGS] - = { qualifier_none, qualifier_unsigned, qualifier_none }; -#define TYPES_BINOP_SUS (aarch64_types_binop_sus_qualifiers) -static enum aarch64_type_qualifiers aarch64_types_binopp_qualifiers[SIMD_MAX_BUILTIN_ARGS] = { qualifier_poly, qualifier_poly, qualifier_poly }; #define TYPES_BINOPP (aarch64_types_binopp_qualifiers) @@ -181,6 +173,10 @@ aarch64_types_shift_to_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS] = { qualifier_unsigned, qualifier_none, qualifier_immediate }; #define TYPES_SHIFTIMM_USS (aarch64_types_shift_to_unsigned_qualifiers) static enum aarch64_type_qualifiers +aarch64_types_fcvt_from_unsigned_qualifiers[SIMD_MAX_BUILTIN_ARGS] + = { qualifier_none, qualifier_unsigned, qualifier_immediate }; +#define TYPES_FCVTIMM_SUS (aarch64_types_fcvt_from_unsigned_qualifiers) +static enum aarch64_type_qualifiers aarch64_types_unsigned_shift_qualifiers[SIMD_MAX_BUILTIN_ARGS] = { qualifier_unsigned, qualifier_unsigned, qualifier_immediate }; #define TYPES_USHIFTIMM (aarch64_types_unsigned_shift_qualifiers) diff --git a/gcc/config/aarch64/aarch64-simd-builtins.def b/gcc/config/aarch64/aarch64-simd-builtins.def index 1332734..02d465b 100644 --- a/gcc/config/aarch64/aarch64-simd-builtins.def +++ b/gcc/config/aarch64/aarch64-simd-builtins.def @@ -447,10 +447,10 @@ BUILTIN_VSDQ_HSI (QUADOP_LANE, sqrdmlsh_laneq, 0) /* Implemented by <FCVT_F2FIXED/FIXED2F:fcvt_fixed_insn><*><*>3. */ - BUILTIN_VSDQ_SDI (BINOP, scvtf, 3) - BUILTIN_VSDQ_SDI (BINOP_SUS, ucvtf, 3) - BUILTIN_VALLF (BINOP, fcvtzs, 3) - BUILTIN_VALLF (BINOP_USS, fcvtzu, 3) + BUILTIN_VSDQ_SDI (SHIFTIMM, scvtf, 3) + BUILTIN_VSDQ_SDI (FCVTIMM_SUS, ucvtf, 3) + BUILTIN_VALLF (SHIFTIMM, fcvtzs, 3) + BUILTIN_VALLF (SHIFTIMM_USS, fcvtzu, 3) /* Implemented by aarch64_rsqrte<mode>. */ BUILTIN_VALLF (UNOP, rsqrte, 0) diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 926f2da..b3ae42b 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4640,8 +4640,8 @@ FCVT_F2FIXED))] "" "@ - <FCVT_F2FIXED:fcvt_fixed_insn>\t%<w1>0, %<s>1, #%2 - <FCVT_F2FIXED:fcvt_fixed_insn>\t%<s>0, %<s>1, #%2" + <FCVT_F2FIXED:fcvt_fixed_insn>\t%<GPF:w1>0, %<GPF:s>1, #%2 + <FCVT_F2FIXED:fcvt_fixed_insn>\t%<GPF:s>0, %<GPF:s>1, #%2" [(set_attr "type" "f_cvtf2i, neon_fp_to_int_<GPF:Vetype>") (set_attr "fp" "yes, *") (set_attr "simd" "*, yes")] @@ -4654,8 +4654,8 @@ FCVT_FIXED2F))] "" "@ - <FCVT_FIXED2F:fcvt_fixed_insn>\t%<s>0, %<w1>1, #%2 - <FCVT_FIXED2F:fcvt_fixed_insn>\t%<s>0, %<s>1, #%2" + <FCVT_FIXED2F:fcvt_fixed_insn>\t%<GPI:v>0, %<GPI:w>1, #%2 + <FCVT_FIXED2F:fcvt_fixed_insn>\t%<GPI:v>0, %<GPI:v>1, #%2" [(set_attr "type" "f_cvti2f, neon_int_to_fp_<GPI:Vetype>") (set_attr "fp" "yes, *") (set_attr "simd" "*, yes")]