diff mbox

[AArch64] Fix __builtin_aarch64_absdi, must not fold to ABS_EXPR

Message ID 546A167C.5090902@arm.com
State New
Headers show

Commit Message

Alan Lawrence Nov. 17, 2014, 3:38 p.m. UTC 
...as the former is defined as returning MIN_VALUE for argument MIN_VALUE, 
whereas the latter is 'undefined', and gcc can optimize "abs(x)>=0" to "true", 
which is wrong for __builtin_aarch64_abs.

There has been much debate here, although not recently - I think the last was 
https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00387.html . However without a 
definite solution, we should at least stop the folding, as otherwise this is a bug.

The complication here is that the folding meant we never expanded the call to 
__builtin_aarch64_absdi, and thus never realized that expanding would cause an 
ICE: gen_absdi2() takes only two parameters (source and dest operand), and has 
no parameter corresponding to the scratch operand in the insn_data; but the code 
in aarch64_simd_expand_args, reads the number of arguments from the insn_data, 
and so tries to get another operand from the call to __builtin_aarch64_absdi, 
causing the ICE. Hence, we have to introduce a SIMD_ARG_SCRATCH, used in 
aarch64_simd_expand_args to skip over the argument.

(There is an alternative way of doing this, i.e. to update the for-loop in 
aarch64_simd_expand_builtin by adding yet another version of 'k'. However, I 
thought there were enough versions already that I really didn't want to add one 
more...)

To go with this, I've renamed qualifier_internal to qualifier_scratch, as it's 
not clear that any non-scratch internal operands (whatever such might be) would 
want the same treatment!

Cross-tested check-gcc on aarch64-none-elf.

Ok for trunk?

gcc/ChangeLog:

	* config/aarch64/aarch64-builtins.c (enum aarch64_type_qualifiers):
	Rename qualifier_internal to qualifier_scratch.
	(aarch64_types_unop_qualifiers, aarch64_init_simd_builtins): Follow
	renaming.
	(builtin_simd_arg): New SIMD_ARG_SCRATCH enum value.
	(aarch64_simd_expand_args): Skip over SIMD_ARG_SCRATCHes.
	(aarch64_simd_expand_builtin): Handle qualifier_scratch.
	(aarch64_fold_builtin): Remove folding of abs.
diff mbox

Patch

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 1af17900785685e4e005710d3bb1743d88a11c16..4cd3d654c0543fad1b88549683222c060c7978d9 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -117,9 +117,10 @@  enum aarch64_type_qualifiers
   qualifier_maybe_immediate = 0x10, /* 1 << 4  */
   /* void foo (...).  */
   qualifier_void = 0x20, /* 1 << 5  */
-  /* Some patterns may have internal operands, this qualifier is an
-     instruction to the initialisation code to skip this operand.  */
-  qualifier_internal = 0x40, /* 1 << 6  */
+  /* Tells the initialisation code to skip this operand when generating
+     the type of the builtin function (and the expander to skip it when
+     expanding a call to said builtin function).  */
+  qualifier_scratch = 0x40, /* 1 << 6  */
   /* Some builtins should use the T_*mode* encoded in a simd_builtin_datum
      rather than using the type of the operand.  */
   qualifier_map_mode = 0x80, /* 1 << 7  */
@@ -140,11 +141,11 @@  typedef struct
   enum aarch64_type_qualifiers *qualifiers;
 } aarch64_simd_builtin_datum;
 
-/*  The qualifier_internal allows generation of a unary builtin from
-    a pattern with a third pseudo-operand such as a match_scratch.  */
+/*  The qualifier_scratch allows generation of a unary builtin from
+    a pattern with a third pseudo-operand i.e. match_scratch.  */
 static enum aarch64_type_qualifiers
 aarch64_types_unop_qualifiers[SIMD_MAX_BUILTIN_ARGS]
-  = { qualifier_none, qualifier_none, qualifier_internal };
+  = { qualifier_none, qualifier_none, qualifier_scratch };
 #define TYPES_UNOP (aarch64_types_unop_qualifiers)
 static enum aarch64_type_qualifiers
 aarch64_types_unopu_qualifiers[SIMD_MAX_BUILTIN_ARGS]
@@ -791,7 +792,7 @@  aarch64_init_simd_builtins (void)
 	    type_signature[arg_num] = 's';
 
 	  /* Skip an internal operand for vget_{low, high}.  */
-	  if (qualifiers & qualifier_internal)
+	  if (qualifiers & qualifier_scratch)
 	    continue;
 
 	  /* Some builtins have different user-facing types
@@ -899,6 +900,7 @@  typedef enum
 {
   SIMD_ARG_COPY_TO_REG,
   SIMD_ARG_CONSTANT,
+  SIMD_ARG_SCRATCH,
   SIMD_ARG_STOP
 } builtin_simd_arg;
 
@@ -919,13 +921,13 @@  aarch64_simd_expand_args (rtx target, int icode, int have_retval,
 	  || !(*insn_data[icode].operand[0].predicate) (target, tmode)))
     target = gen_reg_rtx (tmode);
 
-  for (;;)
+  for (;; args++)
     {
-      builtin_simd_arg thisarg = args[argc];
+      builtin_simd_arg thisarg = *args;
 
       if (thisarg == SIMD_ARG_STOP)
 	break;
-      else
+      else if (thisarg != SIMD_ARG_SCRATCH)
 	{
 	  arg[argc] = CALL_EXPR_ARG (exp, argc);
 	  op[argc] = expand_normal (arg[argc]);
@@ -950,6 +952,7 @@  aarch64_simd_expand_args (rtx target, int icode, int have_retval,
 	      break;
 
 	    case SIMD_ARG_STOP:
+	    case SIMD_ARG_SCRATCH:
 	      gcc_unreachable ();
 	    }
 
@@ -1061,6 +1064,8 @@  aarch64_simd_expand_builtin (int fcode, tree exp, rtx target)
 		(arg, insn_data[icode].operand[operands_k].mode));
 	  args[k] = op_const_int_p ? SIMD_ARG_CONSTANT : SIMD_ARG_COPY_TO_REG;
 	}
+      else if (d->qualifiers[qualifiers_k] & qualifier_scratch)
+        args[k] = SIMD_ARG_SCRATCH;
       else
 	args[k] = SIMD_ARG_COPY_TO_REG;
 
@@ -1317,9 +1322,6 @@  aarch64_fold_builtin (tree fndecl, int n_args ATTRIBUTE_UNUSED, tree *args,
 
   switch (fcode)
     {
-      BUILTIN_VALLDI (UNOP, abs, 2)
-	return fold_build1 (ABS_EXPR, type, args[0]);
-	break;
       BUILTIN_VALLDI (BINOP, cmge, 0)
 	return fold_build2 (GE_EXPR, type, args[0], args[1]);
 	break;