diff mbox

[AArch64] Fixup to fcvt patterns added in r237200

Message ID 1465561779-24588-1-git-send-email-james.greenhalgh@arm.com
State New
Headers show

Commit Message

James Greenhalgh June 10, 2016, 12:29 p.m. UTC
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.

Comments

Christophe Lyon June 17, 2016, 2:25 p.m. UTC | #1
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.

>
James Greenhalgh June 17, 2016, 2:44 p.m. UTC | #2
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
Christophe Lyon June 17, 2016, 2:53 p.m. UTC | #3
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

>
James Greenhalgh June 20, 2016, 11:07 a.m. UTC | #4
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.

>
Richard Earnshaw (lists) June 20, 2016, 1:24 p.m. UTC | #5
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 mbox

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")]