[PATCH][AArch64] Fix FAIL: gcc.target/aarch64/target_attr_crypto_ice_1.c (internal compiler error)

[PATCH][AArch64] Fix FAIL: gcc.target/aarch64/target_attr_crypto_ice_1.c (internal compiler error)

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

Hi all,

This fixes the ICE exposed by Alexandre's patch (https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00873.html)
The solution I came up with is to re-layout the parameter decls not during expansion time (when RTL has already
been allocated to SSA names) but in TARGET_SET_CURRENT_FUNCTION which is called much earlier before that and is
used when setting cfun. This way we reach expand with the proper vector modes registered for the param decls
and all seems to work ok.

The aarch64-builtins.c workaround that I initially introduced in https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02012.html
are partially reverted (at least the re-laying out parts).

The patch fixes the target_attr_crypto_ice_1.c ICE but I'd like to add a second derived testcase that
tests a different expansion path and it has proved useful in writing this patch.

Bootstrapped and tested on aarch64.

Ok for trunk?

Thanks,
Kyrill

2015-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.c (aarch64_set_current_function):
     Re-layout any vector parameters have non-simd layout.
     * config/aarch64/aarch64-builtins.c (aarch64_relayout_simd_param):
     Delete.
     (aarch64_simd_expand_args): Delete call to the above.

2015-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/target_attr_crypto_ice_2.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64-target-attr-ice.patch --]
[-- Type: text/x-patch; name=aarch64-target-attr-ice.patch, Size: 3477 bytes --]

commit 94093e43f5bc91f3afa1002f41dcd423e8db3237
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Aug 18 17:02:26 2015 +0100

    [AArch64] Re-layout vector parameter DECLs in TARGET_SET_CURRENT_FUNCTION

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 0f4f2b9..e3a90b5 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -886,30 +886,6 @@ typedef enum
   SIMD_ARG_STOP
 } builtin_simd_arg;
 
-/* Relayout the decl of a function arg.  Keep the RTL component the same,
-   as varasm.c ICEs.  It doesn't like reinitializing the RTL
-   on PARM decls.  Something like this needs to be done when compiling a
-   file without SIMD and then tagging a function with +simd and using SIMD
-   intrinsics in there.  The types will have been laid out assuming no SIMD,
-   so we want to re-lay them out.  */
-
-static void
-aarch64_relayout_simd_param (tree arg)
-{
-  tree argdecl = arg;
-  if (TREE_CODE (argdecl) == SSA_NAME)
-    argdecl = SSA_NAME_VAR (argdecl);
-
-  if (argdecl
-      && (TREE_CODE (argdecl) == PARM_DECL
-	  || TREE_CODE (argdecl) == VAR_DECL))
-    {
-      rtx rtl = NULL_RTX;
-      rtl = DECL_RTL_IF_SET (argdecl);
-      relayout_decl (argdecl);
-      SET_DECL_RTL (argdecl, rtl);
-    }
-}
 
 static rtx
 aarch64_simd_expand_args (rtx target, int icode, int have_retval,
@@ -940,7 +916,6 @@ aarch64_simd_expand_args (rtx target, int icode, int have_retval,
 	{
 	  tree arg = CALL_EXPR_ARG (exp, opc - have_retval);
 	  enum machine_mode mode = insn_data[icode].operand[opc].mode;
-	  aarch64_relayout_simd_param (arg);
 	  op[opc] = expand_normal (arg);
 
 	  switch (thisarg)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 217b4d7..b16e511 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -8073,6 +8073,23 @@ aarch64_set_current_function (tree fndecl)
 	      = save_target_globals_default_opts ();
 	}
     }
+
+  if (!fndecl)
+    return;
+
+  /* If we turned on SIMD make sure that any vector parameters are re-laid out
+     so that they use proper vector modes.  */
+  if (TARGET_SIMD)
+    {
+      tree parms = DECL_ARGUMENTS (fndecl);
+      for (; parms && parms != void_list_node; parms = TREE_CHAIN (parms))
+	{
+	  if (TREE_CODE (parms) == PARM_DECL
+	      && VECTOR_TYPE_P (TREE_TYPE (parms))
+	      && DECL_MODE (parms) != TYPE_MODE (TREE_TYPE (parms)))
+	    relayout_decl (parms);
+	}
+    }
 }
 
 /* Enum describing the various ways we can handle attributes.
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
new file mode 100644
index 0000000..d6e7b68
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_crypto_ice_2.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mcpu=thunderx+nofp" } */
+
+/* Make sure that we don't ICE when dealing with vector parameters
+   in a simd-tagged function within a non-simd translation unit.  */
+
+#pragma GCC push_options
+#pragma GCC target ("+nothing+simd")
+typedef unsigned int __uint32_t;
+typedef __uint32_t uint32_t ;
+typedef __Uint32x4_t uint32x4_t;
+#pragma GCC pop_options
+
+
+__attribute__ ((target ("cpu=cortex-a57")))
+uint32x4_t
+foo (uint32x4_t a, uint32_t b, uint32x4_t c)
+{
+  return c;
+}

Re: [PATCH][AArch64] Fix FAIL: gcc.target/aarch64/target_attr_crypto_ice_1.c (internal compiler error)

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01084.html

Thanks,
Kyrill

On 19/08/15 14:41, Kyrill Tkachov wrote:
> Hi all,
>
> This fixes the ICE exposed by Alexandre's patch (https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00873.html)
> The solution I came up with is to re-layout the parameter decls not during expansion time (when RTL has already
> been allocated to SSA names) but in TARGET_SET_CURRENT_FUNCTION which is called much earlier before that and is
> used when setting cfun. This way we reach expand with the proper vector modes registered for the param decls
> and all seems to work ok.
>
> The aarch64-builtins.c workaround that I initially introduced in https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02012.html
> are partially reverted (at least the re-laying out parts).
>
> The patch fixes the target_attr_crypto_ice_1.c ICE but I'd like to add a second derived testcase that
> tests a different expansion path and it has proved useful in writing this patch.
>
> Bootstrapped and tested on aarch64.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2015-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       * config/aarch64/aarch64.c (aarch64_set_current_function):
>       Re-layout any vector parameters have non-simd layout.
>       * config/aarch64/aarch64-builtins.c (aarch64_relayout_simd_param):
>       Delete.
>       (aarch64_simd_expand_args): Delete call to the above.
>
> 2015-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       * gcc.target/aarch64/target_attr_crypto_ice_2.c: New test.

Re: [PATCH][AArch64] Fix FAIL: gcc.target/aarch64/target_attr_crypto_ice_1.c (internal compiler error)

[James Greenhalgh]

On Wed, Aug 19, 2015 at 02:41:35PM +0100, Kyrill Tkachov wrote:
> Hi all,
> 
> This fixes the ICE exposed by Alexandre's patch (https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00873.html)
> The solution I came up with is to re-layout the parameter decls not during expansion time (when RTL has already
> been allocated to SSA names) but in TARGET_SET_CURRENT_FUNCTION which is called much earlier before that and is
> used when setting cfun. This way we reach expand with the proper vector modes registered for the param decls
> and all seems to work ok.
> 
> The aarch64-builtins.c workaround that I initially introduced in https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02012.html
> are partially reverted (at least the re-laying out parts).
> 
> The patch fixes the target_attr_crypto_ice_1.c ICE but I'd like to add a second derived testcase that
> tests a different expansion path and it has proved useful in writing this patch.
> 
> Bootstrapped and tested on aarch64.
> 
> Ok for trunk?

OK.

Thanks,
James

> 2015-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>      * config/aarch64/aarch64.c (aarch64_set_current_function):
>      Re-layout any vector parameters have non-simd layout.
>      * config/aarch64/aarch64-builtins.c (aarch64_relayout_simd_param):
>      Delete.
>      (aarch64_simd_expand_args): Delete call to the above.
> 
> 2015-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>      * gcc.target/aarch64/target_attr_crypto_ice_2.c: New test.