public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][AArch64][11/14] Re-layout SIMD builtin types on builtin expansion
@ 2015-07-16 15:21 Kyrill Tkachov
  2015-07-21 17:14 ` Kyrill Tkachov
  0 siblings, 1 reply; 5+ messages in thread
From: Kyrill Tkachov @ 2015-07-16 15:21 UTC (permalink / raw)
  To: GCC Patches; +Cc: Marcus Shawcroft, Richard Earnshaw, James Greenhalgh

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

Hi all,

This patch fixes an ICE that I encountered while testing the series.
The testcase in the patch ICEs during builtin expansion because the testcase
is compiled with +nofp which means the builtin SIMD types are laid out
according to the nofp rules, but later when a function tagged with +simd
tries to use them assuming they are laid out for SIMD, the ICE occurs.

I've struggled for some time to find a good fix for that.
This is the best I could come up with. During expansion time we take
the decl of the thing being passed to the builtin function and re-lay it.
The majority (all?) of uses of these builtins are only within the intrinsics in arm_neon.h anyway.
This fixes the ICE and doesn't have a negative impact on compile time (not that I could measure, anyway)

This patch also initializes the crc intrinsics unconditionally to handle the case where a user may compile
a file with +nocrc and then have a function with +crc using an intrinsic.

Bootstrapped and tested on aarch64.

Ok for trunk?

Thanks,
Kyrill

2015-07-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.c (aarch64_option_valid_attribute_p):
     Initialize simd builtins if TARGET_SIMD.
     * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins):
     Make sure that the builtins are initialized only once no matter how
     many times the function is called.
     (aarch64_init_builtins): Unconditionally initialize crc builtins.
     (aarch64_relayout_simd_param): New function.
     (aarch64_simd_expand_args): Use above during argument expansion.
     * config/aarch64/aarch64-c.c (aarch64_pragma_target_parse): Initialize
     simd builtins if TARGET_SIMD.
     * config/aarch64/aarch64-protos.h (aarch64_init_simd_builtins): New
     prototype.
     (aarch64_relayout_simd_types): Likewise.

2015-07-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/target-attr-crypto-ice-1.c: New test.

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

commit 07191e8bbcd3ecbd14d19f0a4296249ba6c2770f
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed May 20 12:02:33 2015 +0100

    [AArch64][11/N] Re-layout SIMD builtin types on builtin expansion

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 294bf9d..df63ea8 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -555,7 +555,7 @@ aarch64_simd_builtin_type (enum machine_mode mode,
   else
     return aarch64_lookup_simd_builtin_type (mode, qualifier_none);
 }
- 
+
 static void
 aarch64_init_simd_builtin_types (void)
 {
@@ -679,11 +679,18 @@ aarch64_init_simd_builtin_scalar_types (void)
 					     "__builtin_aarch64_simd_udi");
 }
 
-static void
+static bool simd_builtins_inited_p = false;
+
+void
 aarch64_init_simd_builtins (void)
 {
   unsigned int i, fcode = AARCH64_SIMD_PATTERN_START;
 
+  if (simd_builtins_inited_p)
+    return;
+
+  simd_builtins_inited_p = true;
+
   aarch64_init_simd_builtin_types ();
 
   /* Strong-typing hasn't been implemented for all AdvSIMD builtin intrinsics.
@@ -846,8 +853,8 @@ aarch64_init_builtins (void)
 
   if (TARGET_SIMD)
     aarch64_init_simd_builtins ();
-  if (TARGET_CRC32)
-    aarch64_init_crc32_builtins ();
+
+  aarch64_init_crc32_builtins ();
 }
 
 tree
@@ -867,6 +874,31 @@ 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 at varasm.c:1324.  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,
 			  tree exp, builtin_simd_arg *args)
@@ -895,6 +927,7 @@ 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.c b/gcc/config/aarch64/aarch64-c.c
index c3798a1..ecc9974 100644
--- a/gcc/config/aarch64/aarch64-c.c
+++ b/gcc/config/aarch64/aarch64-c.c
@@ -179,6 +179,19 @@ aarch64_pragma_target_parse (tree args, tree pop_target)
 
   cpp_opts->warn_unused_macros = saved_warn_unused_macros;
 
+  /* Initialize SIMD builtins if we haven't already.
+     Set current_target_pragma to NULL for the duration so that
+     the builtin initialization code doesn't try to tag the functions
+     being built with the attributes specified by any current pragma, thus
+     going into an infinite recursion.  */
+  if (TARGET_SIMD)
+    {
+      tree saved_current_target_pragma = current_target_pragma;
+      current_target_pragma = NULL;
+      aarch64_init_simd_builtins ();
+      current_target_pragma = saved_current_target_pragma;
+    }
+
   return ret;
 }
 
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 4704736..d1903ff 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -382,6 +382,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
 extern void aarch64_final_prescan_insn (rtx_insn *);
 extern void aarch64_reset_previous_fndecl (void);
 extern void aarch64_cpu_cpp_builtins (cpp_reader *);
+extern void aarch64_init_simd_builtins (void);
+extern void aarch64_relayout_simd_types (void);
 extern void aarch64_register_pragmas (void);
 extern bool
 aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 3faf3c1..f0f3cdc 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -8466,6 +8466,18 @@ aarch64_option_valid_attribute_p (tree fndecl, tree, tree args, int)
   if (ret)
     {
       aarch64_override_options_internal (&global_options);
+      /* Initialize SIMD builtins if we haven't already.
+         Set current_target_pragma to NULL for the duration so that
+         the builtin initialization code doesn't try to tag the functions
+         being built with the attributes specified by any current pragma, thus
+         going into an infinite recursion.  */
+      if (TARGET_SIMD)
+        {
+	  tree saved_current_target_pragma = current_target_pragma;
+	  current_target_pragma = NULL;
+	  aarch64_init_simd_builtins ();
+	  current_target_pragma = saved_current_target_pragma;
+        }
       new_target = build_target_option_node (&global_options);
     }
   else
@@ -8485,7 +8497,6 @@ aarch64_option_valid_attribute_p (tree fndecl, tree, tree args, int)
     }
 
   cl_target_option_restore (&global_options, &cur_target);
-
   if (old_optimize != new_optimize)
     cl_optimization_restore (&global_options,
 			     TREE_OPTIMIZATION (old_optimize));
diff --git a/gcc/testsuite/gcc.target/aarch64/target-attr-crypto-ice-1.c b/gcc/testsuite/gcc.target/aarch64/target-attr-crypto-ice-1.c
new file mode 100644
index 0000000..9048ed0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/target-attr-crypto-ice-1.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mcpu=thunderx+nofp" } */
+
+#include "arm_neon.h"
+
+/* Unless we do something about re-laying out the SIMD builtin types
+   this testcase ICEs during expansion of the crypto builtin.  */
+
+__attribute__((target("cpu=cortex-a57+crypto")))
+uint32x4_t
+test_vsha1cq_u32 (uint32x4_t hash_abcd, uint32_t hash_e, uint32x4_t wk)
+{
+  return vsha1cq_u32 (hash_abcd, hash_e, wk);
+}
+
+/* This one should be compiled for thunderx with no fp.  */
+int
+foo (int a)
+{
+  return a + 5;
+}

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-03 11:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-16 15:21 [PATCH][AArch64][11/14] Re-layout SIMD builtin types on builtin expansion Kyrill Tkachov
2015-07-21 17:14 ` Kyrill Tkachov
2015-07-22 10:46   ` James Greenhalgh
2015-07-24  8:43     ` Kyrill Tkachov
2015-08-03 11:25       ` James Greenhalgh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).