Patchwork Fix ubsan i?86 {add,sub,mul}v<mode>4 patterns

login
register
mail settings
Submitter Jakub Jelinek
Date March 25, 2014, 7:24 a.m.
Message ID <20140325072432.GK1817@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/333303/
State New
Headers show

Comments

Jakub Jelinek - March 25, 2014, 7:24 a.m.
Hi!

The bootstrap-ubsan bootstrap revealed a problem with the
{add,sub,mul}v<mode>4 patterns.  The problem is that they
accept CONST_INT operands (because the instructions do accept immediates),
but to model what the instruction actually does, we need to have both
the value of the operand itself and it's sign extended value to
2x wider mode, but say (sign_extend:DI (const_int 5)) in the pattern is
invalid RTL (found about this because e.g. num_sign_bit_copies returns
bogus return values on (sign_extend:DI (const_int 0)) ).
The following patch attempts to fix this by using two different define_insns
for each, one that accepts everything but VOIDmode operands (i.e. usually
register, memory, some SYMBOL_REFs/LABEL_REFs/CONSTs where we do have mode),
one that accepts only CONST_INTs.  Hopefully at least the combiner will
canonicalize the potential (sign_extend:DI (const_int N)) into
just (const_int N) and thus the *v<mode>4_1 insns would match (plus the
expander expands it that way too).

Bootstrapped/regtested on x86_64-linux and i686-linux, tested with
i686-linux --with-build-config=bootstrap-ubsan bootstrap.  Ok for trunk?

2014-03-25  Jakub Jelinek  <jakub@redhat.com>

	* config/i386/i386.md (addv<mode>4, subv<mode>4, mulv<mode>4): If
	operands[2] is CONST_INT, don't generate (sign_extend (const_int)).
	(*addv<mode>4, *subv<mode>4, *mulv<mode>4): Disallow CONST_INT_P
	operands[2].  Use We constraint instead of <i>.
	(*addv<mode>4_1, *subv<mode>4_1, *mulv<mode>4_1): New insns.
	* config/i386/constraints.md (We): New constraint.


	Jakub
Uros Bizjak - March 25, 2014, 8:12 a.m.
On Tue, Mar 25, 2014 at 8:24 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The bootstrap-ubsan bootstrap revealed a problem with the
> {add,sub,mul}v<mode>4 patterns.  The problem is that they
> accept CONST_INT operands (because the instructions do accept immediates),
> but to model what the instruction actually does, we need to have both
> the value of the operand itself and it's sign extended value to
> 2x wider mode, but say (sign_extend:DI (const_int 5)) in the pattern is
> invalid RTL (found about this because e.g. num_sign_bit_copies returns
> bogus return values on (sign_extend:DI (const_int 0)) ).
> The following patch attempts to fix this by using two different define_insns
> for each, one that accepts everything but VOIDmode operands (i.e. usually
> register, memory, some SYMBOL_REFs/LABEL_REFs/CONSTs where we do have mode),
> one that accepts only CONST_INTs.  Hopefully at least the combiner will
> canonicalize the potential (sign_extend:DI (const_int N)) into
> just (const_int N) and thus the *v<mode>4_1 insns would match (plus the
> expander expands it that way too).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, tested with
> i686-linux --with-build-config=bootstrap-ubsan bootstrap.  Ok for trunk?

It looks to me that you will also need a corresponding predicate,
similar to x86_64_zext_operand that rejects sign-extended VOIDmode
operands where We constraint is used. That will also solve the problem
with combiner, which will be prohibited from combining VOIDmode
operands.

Also, please use curly braces in multi-line preparation statements.

Uros,.

Patch

--- gcc/config/i386/i386.md.jj	2014-03-19 08:14:37.000000000 +0100
+++ gcc/config/i386/i386.md	2014-03-24 15:03:24.899354663 +0100
@@ -5821,10 +5821,11 @@  (define_expand "addv<mode>4"
 		   (eq:CCO (plus:<DWI>
 			      (sign_extend:<DWI>
 				 (match_operand:SWI 1 "nonimmediate_operand"))
-			      (sign_extend:<DWI>
-				 (match_operand:SWI 2 "<general_operand>")))
+			      (match_dup 4))
 			   (sign_extend:<DWI>
-			      (plus:SWI (match_dup 1) (match_dup 2)))))
+			      (plus:SWI (match_dup 1)
+					(match_operand:SWI 2
+					   "<general_operand>")))))
 	      (set (match_operand:SWI 0 "register_operand")
 		   (plus:SWI (match_dup 1) (match_dup 2)))])
    (set (pc) (if_then_else
@@ -5832,7 +5833,11 @@  (define_expand "addv<mode>4"
 	       (label_ref (match_operand 3))
 	       (pc)))]
   ""
-  "ix86_fixup_binary_operands_no_copy (PLUS, <MODE>mode, operands);")
+  "ix86_fixup_binary_operands_no_copy (PLUS, <MODE>mode, operands);
+   if (CONST_INT_P (operands[2]))
+     operands[4] = operands[2];
+   else
+     operands[4] = gen_rtx_SIGN_EXTEND (<DWI>mode, operands[2]);")
 
 (define_insn "*addv<mode>4"
   [(set (reg:CCO FLAGS_REG)
@@ -5840,16 +5845,42 @@  (define_insn "*addv<mode>4"
 		   (sign_extend:<DWI>
 		      (match_operand:SWI 1 "nonimmediate_operand" "%0,0"))
 		   (sign_extend:<DWI>
-		      (match_operand:SWI 2 "<general_operand>" "<g>,<r><i>")))
+		      (match_operand:SWI 2 "<general_operand>" "<r>mWe,<r>We")))
 		(sign_extend:<DWI>
 		   (plus:SWI (match_dup 1) (match_dup 2)))))
    (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>,<r>m")
 	(plus:SWI (match_dup 1) (match_dup 2)))]
-  "ix86_binary_operator_ok (PLUS, <MODE>mode, operands)"
+  "ix86_binary_operator_ok (PLUS, <MODE>mode, operands)
+   && !CONST_INT_P (operands[2])"
   "add{<imodesuffix>}\t{%2, %0|%0, %2}"
   [(set_attr "type" "alu")
    (set_attr "mode" "<MODE>")])
 
+(define_insn "*addv<mode>4_1"
+  [(set (reg:CCO FLAGS_REG)
+	(eq:CCO (plus:<DWI>
+		   (sign_extend:<DWI>
+		      (match_operand:SWI 1 "nonimmediate_operand" "0"))
+		   (match_operand:<DWI> 3 "const_int_operand" "i"))
+		(sign_extend:<DWI>
+		   (plus:SWI (match_dup 1)
+			     (match_operand:SWI 2 "x86_64_immediate_operand"
+						  "<i>")))))
+   (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
+	(plus:SWI (match_dup 1) (match_dup 2)))]
+  "ix86_binary_operator_ok (PLUS, <MODE>mode, operands)
+   && CONST_INT_P (operands[2])
+   && INTVAL (operands[2]) == INTVAL (operands[3])"
+  "add{<imodesuffix>}\t{%2, %0|%0, %2}"
+  [(set_attr "type" "alu")
+   (set_attr "mode" "<MODE>")
+   (set (attr "length_immediate")
+	(cond [(match_test "IN_RANGE (INTVAL (operands[2]), -128, 127)")
+		  (const_string "1")
+	       (match_test "<MODE_SIZE> == 8")
+		  (const_string "4")]
+	      (const_string "<MODE_SIZE>")))])
+
 ;; The lea patterns for modes less than 32 bits need to be matched by
 ;; several insns converted to real lea by splitters.
 
@@ -6093,10 +6124,11 @@  (define_expand "subv<mode>4"
 		   (eq:CCO (minus:<DWI>
 			      (sign_extend:<DWI>
 				 (match_operand:SWI 1 "nonimmediate_operand"))
-			      (sign_extend:<DWI>
-				 (match_operand:SWI 2 "<general_operand>")))
+			      (match_dup 4))
 			   (sign_extend:<DWI>
-			      (minus:SWI (match_dup 1) (match_dup 2)))))
+			      (minus:SWI (match_dup 1)
+					 (match_operand:SWI 2
+					    "<general_operand>")))))
 	      (set (match_operand:SWI 0 "register_operand")
 		   (minus:SWI (match_dup 1) (match_dup 2)))])
    (set (pc) (if_then_else
@@ -6104,7 +6136,11 @@  (define_expand "subv<mode>4"
 	       (label_ref (match_operand 3))
 	       (pc)))]
   ""
-  "ix86_fixup_binary_operands_no_copy (MINUS, <MODE>mode, operands);")
+  "ix86_fixup_binary_operands_no_copy (MINUS, <MODE>mode, operands);
+   if (CONST_INT_P (operands[2]))
+     operands[4] = operands[2];
+   else
+     operands[4] = gen_rtx_SIGN_EXTEND (<DWI>mode, operands[2]);")
 
 (define_insn "*subv<mode>4"
   [(set (reg:CCO FLAGS_REG)
@@ -6112,16 +6148,42 @@  (define_insn "*subv<mode>4"
 		   (sign_extend:<DWI>
 		      (match_operand:SWI 1 "nonimmediate_operand" "0,0"))
 		   (sign_extend:<DWI>
-		      (match_operand:SWI 2 "<general_operand>" "<r><i>,<r>m")))
+		      (match_operand:SWI 2 "<general_operand>" "<r>We,<r>m")))
 		(sign_extend:<DWI>
 		   (minus:SWI (match_dup 1) (match_dup 2)))))
    (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m,<r>")
 	(minus:SWI (match_dup 1) (match_dup 2)))]
-  "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)"
+  "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)
+   && !CONST_INT_P (operands[2])"
   "sub{<imodesuffix>}\t{%2, %0|%0, %2}"
   [(set_attr "type" "alu")
    (set_attr "mode" "<MODE>")])
 
+(define_insn "*subv<mode>4_1"
+  [(set (reg:CCO FLAGS_REG)
+	(eq:CCO (minus:<DWI>
+		   (sign_extend:<DWI>
+		      (match_operand:SWI 1 "nonimmediate_operand" "0"))
+		   (match_operand:<DWI> 3 "const_int_operand" "i"))
+		(sign_extend:<DWI>
+		   (minus:SWI (match_dup 1)
+			      (match_operand:SWI 2 "x86_64_immediate_operand"
+						   "<i>")))))
+   (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
+	(minus:SWI (match_dup 1) (match_dup 2)))]
+  "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)
+   && CONST_INT_P (operands[2])
+   && INTVAL (operands[2]) == INTVAL (operands[3])"
+  "sub{<imodesuffix>}\t{%2, %0|%0, %2}"
+  [(set_attr "type" "alu")
+   (set_attr "mode" "<MODE>")
+   (set (attr "length_immediate")
+	(cond [(match_test "IN_RANGE (INTVAL (operands[2]), -128, 127)")
+		  (const_string "1")
+	       (match_test "<MODE_SIZE> == 8")
+		  (const_string "4")]
+	      (const_string "<MODE_SIZE>")))])
+
 (define_insn "*sub<mode>_3"
   [(set (reg FLAGS_REG)
 	(compare (match_operand:SWI 1 "nonimmediate_operand" "0,0")
@@ -6442,52 +6504,96 @@  (define_expand "mulv<mode>4"
 		   (eq:CCO (mult:<DWI>
 			      (sign_extend:<DWI>
 				 (match_operand:SWI48 1 "register_operand"))
-			      (sign_extend:<DWI>
-				 (match_operand:SWI48 2 "<general_operand>")))
+			      (match_dup 4))
 			   (sign_extend:<DWI>
-			      (mult:SWI48 (match_dup 1) (match_dup 2)))))
+			      (mult:SWI48 (match_dup 1)
+					  (match_operand:SWI48 2
+					     "<general_operand>")))))
 	      (set (match_operand:SWI48 0 "register_operand")
 		   (mult:SWI48 (match_dup 1) (match_dup 2)))])
    (set (pc) (if_then_else
 	       (eq (reg:CCO FLAGS_REG) (const_int 0))
 	       (label_ref (match_operand 3))
-	       (pc)))])
+	       (pc)))]
+  ""
+  "if (CONST_INT_P (operands[2]))
+     operands[4] = operands[2];
+   else
+     operands[4] = gen_rtx_SIGN_EXTEND (<DWI>mode, operands[2]);")
 
 (define_insn "*mulv<mode>4"
   [(set (reg:CCO FLAGS_REG)
 	(eq:CCO (mult:<DWI>
 		   (sign_extend:<DWI>
-		      (match_operand:SWI 1 "nonimmediate_operand" "%rm,rm,0"))
+		      (match_operand:SWI48 1 "nonimmediate_operand" "%rm,0"))
 		   (sign_extend:<DWI>
-		      (match_operand:SWI 2 "<general_operand>" "K,<i>,mr")))
+		      (match_operand:SWI48 2 "<general_operand>" "We,mr")))
 		(sign_extend:<DWI>
-		   (mult:SWI (match_dup 1) (match_dup 2)))))
-   (set (match_operand:SWI 0 "register_operand" "=r,r,r")
-	(mult:SWI (match_dup 1) (match_dup 2)))]
-  "!(MEM_P (operands[1]) && MEM_P (operands[2]))"
+		   (mult:SWI48 (match_dup 1) (match_dup 2)))))
+   (set (match_operand:SWI48 0 "register_operand" "=r,r")
+	(mult:SWI48 (match_dup 1) (match_dup 2)))]
+  "!(MEM_P (operands[1]) && MEM_P (operands[2]))
+   && !CONST_INT_P (operands[2])"
   "@
    imul{<imodesuffix>}\t{%2, %1, %0|%0, %1, %2}
-   imul{<imodesuffix>}\t{%2, %1, %0|%0, %1, %2}
    imul{<imodesuffix>}\t{%2, %0|%0, %2}"
   [(set_attr "type" "imul")
-   (set_attr "prefix_0f" "0,0,1")
+   (set_attr "prefix_0f" "0,1")
    (set (attr "athlon_decode")
 	(cond [(eq_attr "cpu" "athlon")
 		  (const_string "vector")
-	       (eq_attr "alternative" "1")
+	       (eq_attr "alternative" "0")
 		  (const_string "vector")
-	       (and (eq_attr "alternative" "2")
+	       (and (eq_attr "alternative" "1")
 		    (match_operand 1 "memory_operand"))
 		  (const_string "vector")]
 	      (const_string "direct")))
    (set (attr "amdfam10_decode")
-	(cond [(and (eq_attr "alternative" "0,1")
+	(cond [(and (eq_attr "alternative" "1")
 		    (match_operand 1 "memory_operand"))
 		  (const_string "vector")]
 	      (const_string "direct")))
    (set_attr "bdver1_decode" "direct")
    (set_attr "mode" "<MODE>")])
 
+(define_insn "*mulv<mode>4_1"
+  [(set (reg:CCO FLAGS_REG)
+	(eq:CCO (mult:<DWI>
+		   (sign_extend:<DWI>
+		      (match_operand:SWI48 1 "nonimmediate_operand" "rm,rm"))
+		   (match_operand:<DWI> 3 "const_int_operand" "K,i"))
+		(sign_extend:<DWI>
+		   (mult:SWI48 (match_dup 1)
+			       (match_operand:SWI 2 "x86_64_immediate_operand"
+						    "K,<i>")))))
+   (set (match_operand:SWI48 0 "register_operand" "=r,r")
+	(mult:SWI48 (match_dup 1) (match_dup 2)))]
+  "!(MEM_P (operands[1]) && MEM_P (operands[2]))
+   && CONST_INT_P (operands[2])
+   && INTVAL (operands[2]) == INTVAL (operands[3])"
+  "@
+   imul{<imodesuffix>}\t{%2, %1, %0|%0, %1, %2}
+   imul{<imodesuffix>}\t{%2, %1, %0|%0, %1, %2}"
+  [(set_attr "type" "imul")
+   (set (attr "athlon_decode")
+	(cond [(eq_attr "cpu" "athlon")
+		  (const_string "vector")
+	       (eq_attr "alternative" "1")
+		  (const_string "vector")]
+	      (const_string "direct")))
+   (set (attr "amdfam10_decode")
+	(cond [(match_operand 1 "memory_operand")
+		  (const_string "vector")]
+	      (const_string "direct")))
+   (set_attr "bdver1_decode" "direct")
+   (set_attr "mode" "<MODE>")
+   (set (attr "length_immediate")
+	(cond [(match_test "IN_RANGE (INTVAL (operands[2]), -128, 127)")
+		  (const_string "1")
+	       (match_test "<MODE_SIZE> == 8")
+		  (const_string "4")]
+	      (const_string "<MODE_SIZE>")))])
+
 (define_expand "<u>mul<mode><dwi>3"
   [(parallel [(set (match_operand:<DWI> 0 "register_operand")
 		   (mult:<DWI>
--- gcc/config/i386/constraints.md.jj	2014-01-31 22:22:10.000000000 +0100
+++ gcc/config/i386/constraints.md	2014-03-24 15:01:08.831074223 +0100
@@ -220,6 +220,13 @@  (define_constraint "e"
 ;; We use W prefix to denote any number of
 ;; constant-or-symbol-reference constraints
 
+(define_constraint "We"
+  "32-bit signed integer constant, or a symbolic reference known
+   to fit that range (for sign-extending conversion operations that
+   require non-VOIDmode immediate operands)."
+  (and (match_operand 0 "x86_64_immediate_operand")
+       (match_test "GET_MODE (op) != VOIDmode")))
+
 (define_constraint "Wz"
   "32-bit unsigned integer constant, or a symbolic reference known
    to fit that range (for zero-extending conversion operations that