diff mbox series

Fix vec_{add,sub} rs6000_gimple_fold_builtin folding (PR target/88234)

Message ID 20181129140022.GI12380@tucnak
State New
Headers show
Series Fix vec_{add,sub} rs6000_gimple_fold_builtin folding (PR target/88234) | expand

Commit Message

Jakub Jelinek Nov. 29, 2018, 2 p.m. UTC 
Hi!

vec_add/sub of with vector unsigned args is lowered to a builtin which
has vector signed args and therefore if not -fwrapv it is undefined if
signed integer overflow occurs in those vectors.

The following patch fixes it to make sure that those builtins are folded
to PLUS/MINUS_EXPR done on unsigned vectors instead, so there is no UB.
If it makes it through to RTL expansion, it makes no difference, but
for UBSan it matters a lot and also I'd say if e.g. we'd extract just one
scalar from the resulting vector, we'd optimize it just to a scalar +/- and
could very well optimize based on lack of UB.

I've looked at a couple of other builtins, but e.g. with vec_mul* couldn't
trigger anything problematic.

Bootstrapped/regtested on powerpc64{,le}-linux, ok for trunk?

2018-11-29  Jakub Jelinek  <jakub@redhat.com>

	PR target/88234
	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): For
	vec_add and vec_sub builtins, perform PLUS_EXPR or MINUS_EXPR
	in unsigned_type_for instead of vector integral type where overflow
	doesn't wrap.

	* gcc.dg/ubsan/pr88234.c: New test.


	Jakub

Comments

Segher Boessenkool Nov. 29, 2018, 2:14 p.m. UTC | #1 
Hi Jakub,

On Thu, Nov 29, 2018 at 03:00:22PM +0100, Jakub Jelinek wrote:
> vec_add/sub of with vector unsigned args is lowered to a builtin which
> has vector signed args and therefore if not -fwrapv it is undefined if
> signed integer overflow occurs in those vectors.
> 
> The following patch fixes it to make sure that those builtins are folded
> to PLUS/MINUS_EXPR done on unsigned vectors instead, so there is no UB.
> If it makes it through to RTL expansion, it makes no difference, but
> for UBSan it matters a lot and also I'd say if e.g. we'd extract just one
> scalar from the resulting vector, we'd optimize it just to a scalar +/- and
> could very well optimize based on lack of UB.
> 
> I've looked at a couple of other builtins, but e.g. with vec_mul* couldn't
> trigger anything problematic.
> 
> Bootstrapped/regtested on powerpc64{,le}-linux, ok for trunk?

Okay for trunk, and backports too if you want any.  Thanks!


Segher


> 2018-11-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/88234
> 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): For
> 	vec_add and vec_sub builtins, perform PLUS_EXPR or MINUS_EXPR
> 	in unsigned_type_for instead of vector integral type where overflow
> 	doesn't wrap.
> 
> 	* gcc.dg/ubsan/pr88234.c: New test.
diff mbox series

Patch

--- gcc/config/rs6000/rs6000.c.jj	2018-11-29 08:41:29.753806139 +0100
+++ gcc/config/rs6000/rs6000.c	2018-11-29 11:39:04.783862074 +0100
@@ -15371,6 +15371,7 @@  rs6000_gimple_fold_builtin (gimple_stmt_
   enum rs6000_builtins fn_code
     = (enum rs6000_builtins) DECL_FUNCTION_CODE (fndecl);
   tree arg0, arg1, lhs, temp;
+  enum tree_code bcode;
   gimple *g;
 
   size_t uns_fncode = (size_t) fn_code;
@@ -15409,10 +15410,32 @@  rs6000_gimple_fold_builtin (gimple_stmt_
     case P8V_BUILTIN_VADDUDM:
     case ALTIVEC_BUILTIN_VADDFP:
     case VSX_BUILTIN_XVADDDP:
+      bcode = PLUS_EXPR;
+    do_binary:
       arg0 = gimple_call_arg (stmt, 0);
       arg1 = gimple_call_arg (stmt, 1);
       lhs = gimple_call_lhs (stmt);
-      g = gimple_build_assign (lhs, PLUS_EXPR, arg0, arg1);
+      if (INTEGRAL_TYPE_P (TREE_TYPE (TREE_TYPE (lhs)))
+	  && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (TREE_TYPE (lhs))))
+	{
+	  /* Ensure the binary operation is performed in a type
+	     that wraps if it is integral type.  */
+	  gimple_seq stmts = NULL;
+	  tree type = unsigned_type_for (TREE_TYPE (lhs));
+	  tree uarg0 = gimple_build (&stmts, VIEW_CONVERT_EXPR,
+				     type, arg0);
+	  tree uarg1 = gimple_build (&stmts, VIEW_CONVERT_EXPR,
+				     type, arg1);
+	  tree res = gimple_build (&stmts, gimple_location (stmt), bcode,
+				   type, uarg0, uarg1);
+	  gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
+	  g = gimple_build_assign (lhs, VIEW_CONVERT_EXPR,
+				   build1 (VIEW_CONVERT_EXPR,
+					   TREE_TYPE (lhs), res));
+	  gsi_replace (gsi, g, true);
+	  return true;
+	}
+      g = gimple_build_assign (lhs, bcode, arg0, arg1);
       gimple_set_location (g, gimple_location (stmt));
       gsi_replace (gsi, g, true);
       return true;
@@ -15424,13 +15447,8 @@  rs6000_gimple_fold_builtin (gimple_stmt_
     case P8V_BUILTIN_VSUBUDM:
     case ALTIVEC_BUILTIN_VSUBFP:
     case VSX_BUILTIN_XVSUBDP:
-      arg0 = gimple_call_arg (stmt, 0);
-      arg1 = gimple_call_arg (stmt, 1);
-      lhs = gimple_call_lhs (stmt);
-      g = gimple_build_assign (lhs, MINUS_EXPR, arg0, arg1);
-      gimple_set_location (g, gimple_location (stmt));
-      gsi_replace (gsi, g, true);
-      return true;
+      bcode = MINUS_EXPR;
+      goto do_binary;
     case VSX_BUILTIN_XVMULSP:
     case VSX_BUILTIN_XVMULDP:
       arg0 = gimple_call_arg (stmt, 0);
--- gcc/testsuite/gcc.dg/ubsan/pr88234.c.jj	2018-11-29 12:13:06.879735598 +0100
+++ gcc/testsuite/gcc.dg/ubsan/pr88234.c	2018-11-29 12:13:54.594937165 +0100
@@ -0,0 +1,29 @@ 
+/* PR target/88234 */
+/* { dg-do run { target { powerpc*-*-* && vmx_hw } } } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-options "-fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow -O2 -maltivec" } */
+
+#include <altivec.h>
+
+__attribute__((noipa)) vector unsigned int
+f1 (vector unsigned int x, vector unsigned int y)
+{
+  return vec_add (x, y);
+}
+
+__attribute__((noipa)) vector unsigned int
+f2 (vector unsigned int x, vector unsigned int y)
+{
+  return vec_sub (x, y);
+}
+
+int
+main ()
+{
+  vector unsigned int x = { __INT_MAX__, -__INT_MAX__, __INT_MAX__ - 3, -__INT_MAX__ + 4 };
+  vector unsigned int y = { 1, -1, 4, -5 };
+  vector unsigned int z = f1 (x, y);
+  f2 (z, x);
+  f2 (z, y);
+  return 0;
+}