diff mbox

[AArch64] PR 61749: Do not ICE in lane intrinsics when passed non-constant lane number

Message ID 54097D4F.9010609@arm.com
State New
Headers show

Commit Message

Kyrylo Tkachov Sept. 5, 2014, 9:07 a.m. UTC
Hi all,

As the PR says we currently ICE in aarch64_simd_lane_bounds when processing

#include "arm_neon.h"

int32x4_t foo (int32x4_t a, int16x4_t b, int16x4_t c, int d)
{
   return vqdmlal_lane_s16 (a, b, c, d);
}

This code is invalid since the lane argument (d) should be a 
compile-time constant. This can be fixed by setting the qualifier for 
the 4th argument for these intrinsics to qualifier_immediate so that the 
expansion code in aarch64-builtins.c can detect that and emit the 
appropriate message.

This, however, is not enough by itself. We will emit the error but then 
proceed anyway and ICE. From looking around other backends (and rs6000 
in particular), the correct thing to do in these cases is to return 
const0_rtx to signify that a user input error occured. This patch does 
that and also makes sure we hit gcc_unreachable () instead of returning 
NULL_RTX when the requested builtin to expand cannot be found. This is 
the correct thing to do because returning NULL_RTX is apparently just 
the way to show that the builtin does not return a result (e.g. for void 
builtins).

Before this patch on the above code we would get:

$BUILD/install/lib/gcc/aarch64-none-elf/4.10.0/include/arm_neon.h: In 
function 'foo':
$BUILD/install/lib/gcc/aarch64-none-elf/4.10.0/include/arm_neon.h:19294:10: 
internal compiler error: in aarch64_simd_lane_bounds, at 
config/aarch64/aarch64.c:7715
    return __builtin_aarch64_sqdmlal_lanev4hi (__a, __b, __c, __d);
           ^
0xc608d0 aarch64_simd_lane_bounds(rtx_def*, long, long)
     $SRC/gcc/config/aarch64/aarch64.c:7715
0xcb0221 gen_aarch64_sqdmlal_lanev4hi(rtx_def*, rtx_def*, rtx_def*, 
rtx_def*, rtx_def*)
$SRC/gcc/config/aarch64/aarch64-simd.md:3015
0xc65b7f insn_gen_fn::operator()(rtx_def*, rtx_def*, rtx_def*, rtx_def*, 
rtx_def*) const
$SRC/src/gcc/gcc/recog.h:311
0xc65b7f aarch64_simd_expand_args
$SRC/gcc/config/aarch64/aarch64-builtins.c:888
0xc66318 aarch64_simd_expand_builtin(int, tree_node*, rtx_def*)
$SRC/gcc/config/aarch64/aarch64-builtins.c:990
0xc66968 aarch64_expand_builtin(tree_node*, rtx_def*, rtx_def*, 
machine_mode, int)
etc...


Now we get the more helpful:
build-aarch64/install/lib/gcc/aarch64-none-elf/4.10.0/include/arm_neon.h:19371:10: 
error: incompatible type for argument 4, expected 'const int'
    return __builtin_aarch64_sqdmlal_lanev4hi (__a, __b, __c, __d);

As for the testcase, we want to check that we give an error but do not 
ICE. The dg-excess-errors directive is the closest I've found to that.
The test appears as an expected fail. If, however, we were to ICE it 
would appear as an unexpected failure, which is what we would want.

Tested on aarch64-none-elf and bootstrapped on aarch64-linux.

Ok for trunk?

2014-09-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/61749
     * config/aarch64/aarch64-builtins.c (aarch64_types_quadop_qualifiers):
     Use qualifier_immediate for last operand.  Rename to...
     (aarch64_types_ternop_lane_qualifiers): ... This.
     (TYPES_QUADOP): Rename to...
     (TYPES_TERNOP_LANE): ... This.
     (aarch64_simd_expand_args): Return const0_rtx when encountering user
     error.  Change return of 0 to return of NULL_RTX.
     (aarch64_crc32_expand_builtin): Likewise.
     (aarch64_expand_builtin): Return NULL_RTX instead of 0.
     ICE when expanding unknown builtin.
     * config/aarch64/aarch64-simd-builtins.def (sqdmlal_lane): Use
     TERNOP_LANE qualifiers.
     (sqdmlsl_lane): Likewise.
     (sqdmlal_laneq): Likewise.
     (sqdmlsl_laneq): Likewise.
     (sqdmlal2_lane): Likewise.
     (sqdmlsl2_lane): Likewise.
     (sqdmlal2_laneq): Likewise.
     (sqdmlsl2_laneq): Likewise.

2014-09-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/61749
     * gcc.target/aarch64/vqdml_lane_intrinsics-bad_1.c: New test.

Comments

Marcus Shawcroft Sept. 9, 2014, 9:38 a.m. UTC | #1
On 5 September 2014 10:07, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:

> Ok for trunk?
>
> 2014-09-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/61749
>     * config/aarch64/aarch64-builtins.c (aarch64_types_quadop_qualifiers):
>     Use qualifier_immediate for last operand.  Rename to...
>     (aarch64_types_ternop_lane_qualifiers): ... This.
>     (TYPES_QUADOP): Rename to...
>     (TYPES_TERNOP_LANE): ... This.
>     (aarch64_simd_expand_args): Return const0_rtx when encountering user
>     error.  Change return of 0 to return of NULL_RTX.
>     (aarch64_crc32_expand_builtin): Likewise.
>     (aarch64_expand_builtin): Return NULL_RTX instead of 0.
>     ICE when expanding unknown builtin.
>     * config/aarch64/aarch64-simd-builtins.def (sqdmlal_lane): Use
>     TERNOP_LANE qualifiers.
>     (sqdmlsl_lane): Likewise.
>     (sqdmlal_laneq): Likewise.
>     (sqdmlsl_laneq): Likewise.
>     (sqdmlal2_lane): Likewise.
>     (sqdmlsl2_lane): Likewise.
>     (sqdmlal2_laneq): Likewise.
>     (sqdmlsl2_laneq): Likewise.
>
> 2014-09-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/61749
>     * gcc.target/aarch64/vqdml_lane_intrinsics-bad_1.c: New test.

OK
/Marcus
diff mbox

Patch

commit 796f7ec499411034d5eb7441b51d0493d6299327
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Aug 6 16:47:29 2014 +0100

    [AArch64] PR target/61749 Fix ICE when passing non-literal lane to some intrinsics

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index ba58a99..16c9329 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -178,10 +178,10 @@  aarch64_types_ternopu_qualifiers[SIMD_MAX_BUILTIN_ARGS]
 #define TYPES_TERNOPU (aarch64_types_ternopu_qualifiers)
 
 static enum aarch64_type_qualifiers
-aarch64_types_quadop_qualifiers[SIMD_MAX_BUILTIN_ARGS]
+aarch64_types_ternop_lane_qualifiers[SIMD_MAX_BUILTIN_ARGS]
   = { qualifier_none, qualifier_none, qualifier_none,
-      qualifier_none, qualifier_none };
-#define TYPES_QUADOP (aarch64_types_quadop_qualifiers)
+      qualifier_none, qualifier_immediate };
+#define TYPES_TERNOP_LANE (aarch64_types_ternop_lane_qualifiers)
 
 static enum aarch64_type_qualifiers
 aarch64_types_getlane_qualifiers[SIMD_MAX_BUILTIN_ARGS]
@@ -907,8 +907,11 @@  aarch64_simd_expand_args (rtx target, int icode, int have_retval,
 	    case SIMD_ARG_CONSTANT:
 	      if (!(*insn_data[icode].operand[argc + have_retval].predicate)
 		  (op[argc], mode[argc]))
+	      {
 		error_at (EXPR_LOCATION (exp), "incompatible type for argument %d, "
 		       "expected %<const int%>", argc + 1);
+		return const0_rtx;
+	      }
 	      break;
 
 	    case SIMD_ARG_STOP:
@@ -975,7 +978,7 @@  aarch64_simd_expand_args (rtx target, int icode, int have_retval,
       }
 
   if (!pat)
-    return 0;
+    return NULL_RTX;
 
   emit_insn (pat);
 
@@ -1071,8 +1074,9 @@  aarch64_crc32_expand_builtin (int fcode, tree exp, rtx target)
     op1 = copy_to_mode_reg (mode1, op1);
 
   pat = GEN_FCN (icode) (target, op0, op1);
-  if (! pat)
-    return 0;
+  if (!pat)
+    return NULL_RTX;
+
   emit_insn (pat);
   return target;
 }
@@ -1124,7 +1128,7 @@  aarch64_expand_builtin (tree exp,
   else if (fcode >= AARCH64_CRC32_BUILTIN_BASE && fcode <= AARCH64_CRC32_BUILTIN_MAX)
     return aarch64_crc32_expand_builtin (fcode, exp, target);
 
-  return NULL_RTX;
+  gcc_unreachable ();
 }
 
 tree
diff --git a/gcc/config/aarch64/aarch64-simd-builtins.def b/gcc/config/aarch64/aarch64-simd-builtins.def
index 4f3bd12..94b81a8 100644
--- a/gcc/config/aarch64/aarch64-simd-builtins.def
+++ b/gcc/config/aarch64/aarch64-simd-builtins.def
@@ -157,16 +157,16 @@ 
   BUILTIN_VSDQ_I (UNOP, sqabs, 0)
   BUILTIN_VSDQ_I (UNOP, sqneg, 0)
 
-  BUILTIN_VSD_HSI (QUADOP, sqdmlal_lane, 0)
-  BUILTIN_VSD_HSI (QUADOP, sqdmlsl_lane, 0)
-  BUILTIN_VSD_HSI (QUADOP, sqdmlal_laneq, 0)
-  BUILTIN_VSD_HSI (QUADOP, sqdmlsl_laneq, 0)
+  BUILTIN_VSD_HSI (TERNOP_LANE, sqdmlal_lane, 0)
+  BUILTIN_VSD_HSI (TERNOP_LANE, sqdmlsl_lane, 0)
+  BUILTIN_VSD_HSI (TERNOP_LANE, sqdmlal_laneq, 0)
+  BUILTIN_VSD_HSI (TERNOP_LANE, sqdmlsl_laneq, 0)
   BUILTIN_VQ_HSI (TERNOP, sqdmlal2, 0)
   BUILTIN_VQ_HSI (TERNOP, sqdmlsl2, 0)
-  BUILTIN_VQ_HSI (QUADOP, sqdmlal2_lane, 0)
-  BUILTIN_VQ_HSI (QUADOP, sqdmlsl2_lane, 0)
-  BUILTIN_VQ_HSI (QUADOP, sqdmlal2_laneq, 0)
-  BUILTIN_VQ_HSI (QUADOP, sqdmlsl2_laneq, 0)
+  BUILTIN_VQ_HSI (TERNOP_LANE, sqdmlal2_lane, 0)
+  BUILTIN_VQ_HSI (TERNOP_LANE, sqdmlsl2_lane, 0)
+  BUILTIN_VQ_HSI (TERNOP_LANE, sqdmlal2_laneq, 0)
+  BUILTIN_VQ_HSI (TERNOP_LANE, sqdmlsl2_laneq, 0)
   BUILTIN_VQ_HSI (TERNOP, sqdmlal2_n, 0)
   BUILTIN_VQ_HSI (TERNOP, sqdmlsl2_n, 0)
   /* Implemented by aarch64_sqdml<SBINQOPS:as>l<mode>.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/vqdml_lane_intrinsics-bad_1.c b/gcc/testsuite/gcc.target/aarch64/vqdml_lane_intrinsics-bad_1.c
new file mode 100644
index 0000000..314a624
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/vqdml_lane_intrinsics-bad_1.c
@@ -0,0 +1,54 @@ 
+/* { dg-do compile } */
+
+#include "arm_neon.h"
+
+int32x4_t
+foo (int32x4_t a, int16x4_t b, int16x4_t c, int d)
+{
+  return vqdmlal_lane_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo1 (int32x4_t a, int16x4_t b, int16x8_t c, int d)
+{
+  return vqdmlal_laneq_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo2 (int32x4_t a, int16x4_t b, int16x4_t c, int d)
+{
+  return vqdmlsl_lane_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo3 (int32x4_t a, int16x4_t b, int16x8_t c, int d)
+{
+  return vqdmlsl_laneq_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo4 (int32x4_t a, int16x8_t b, int16x4_t c, int d)
+{
+  return vqdmlal_high_lane_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo5 (int32x4_t a, int16x8_t b, int16x4_t c, int d)
+{
+  return vqdmlsl_high_lane_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo6 (int32x4_t a, int16x8_t b, int16x8_t c, int d)
+{
+  return vqdmlal_high_laneq_s16 (a, b, c, d);
+}
+
+int32x4_t
+foo7 (int32x4_t a, int16x8_t b, int16x8_t c, int d)
+{
+  return vqdmlsl_high_laneq_s16 (a, b, c, d);
+}
+
+
+/* { dg-excess-errors "incompatible type for argument" } */