| Message ID | 1465561779-24588-1-git-send-email-james.greenhalgh@arm.com |
|---|---|
| State | New |
| Headers | show |
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? > > 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. > LGTM (but I can't approve). Kyrill
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")]
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? 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.