diff mbox

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

Message ID 20140325101734.GQ1817@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek March 25, 2014, 10:17 a.m. UTC 
On Tue, Mar 25, 2014 at 09:12:14AM +0100, Uros Bizjak wrote:
> > 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.

Like this?  Or do you prefer a different name for the predicate?
And, is it ok to use just one predicate for both {QI,HI}mode and
{SI,DI}mode, or should I add separate predicates for "general_operand
where GET_MODE (op) != VOIDmode" and "x86_64_general_operand where
GET_MODE (op) != VOIDmode" and add a mode attribute for that?
If so, what would be your preferred names for the 2 predicates and for the
mode attribute?

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> and x86_64_sext_operand
	predicate instead of <general_operand>.
	(*addv<mode>4_1, *subv<mode>4_1, *mulv<mode>4_1): New insns.
	* config/i386/constraints.md (We): New constraint.
	* config/i386/predicates.md (x86_64_sext_operand): New predicate.



	Jakub

Comments

Uros Bizjak March 25, 2014, 11:39 a.m. UTC | #1 
On Tue, Mar 25, 2014 at 11:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Mar 25, 2014 at 09:12:14AM +0100, Uros Bizjak wrote:
>> > 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.
>
> Like this?  Or do you prefer a different name for the predicate?
> And, is it ok to use just one predicate for both {QI,HI}mode and
> {SI,DI}mode, or should I add separate predicates for "general_operand
> where GET_MODE (op) != VOIDmode" and "x86_64_general_operand where
> GET_MODE (op) != VOIDmode" and add a mode attribute for that?
> If so, what would be your preferred names for the 2 predicates and for the
> mode attribute?

The patch is OK in principle, but we could follow established practice
and use separate predicates - please see general_szext_operand mode
attribute definition.

I propose to use new "general_sext_operand" mode attribute that would
expand to "sext_operand" in QI/HImode case and "x86_64_sext_operand"
in SI/DImode case.

"sext_operand" should be defined similar to IOR part of
x86_64_zext_operand (using "immediate_operand" instead of
x86_64_zext_immediate_operand").

"x86_64_sext_operand" should be defined as "sext_operand", but should
use "x86_64_immediate_operand" in place of "immediate_operand".

(Looking at these definitions, it looks to me that other sign/zero
extend predicates also need to reject VOIDmode immediates, but let's
leave this to 4.9+).

Uros.
Jakub Jelinek March 25, 2014, 7:18 p.m. UTC | #2 
On Tue, Mar 25, 2014 at 04:06:40PM +0100, Jakub Jelinek wrote:
> On Tue, Mar 25, 2014 at 12:39:18PM +0100, Uros Bizjak wrote:
> > The patch is OK in principle, but we could follow established practice
> > and use separate predicates - please see general_szext_operand mode
> > attribute definition.
> 
> So like this?  I've tried to use non-VOIDmode of the predicates that were
> used previously (i.e. general_operand or x86_64_general_operand).
> 
> 2014-03-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* config/i386/i386.md (general_sext_operand): New mode attr.
> 	(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> and <general_sext_operand>
> 	predicate instead of <general_operand>.
> 	(*addv<mode>4_1, *subv<mode>4_1, *mulv<mode>4_1): New insns.
> 	* config/i386/constraints.md (We): New constraint.
> 	* config/i386/predicates.md (x86_64_sext_operand,
> 	sext_operand): New predicates.

Now successfully bootstrapped/regtested on x86_64-linux and i686-linux.

	Jakub
Uros Bizjak March 25, 2014, 7:52 p.m. UTC | #3 
On Tue, Mar 25, 2014 at 8:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Mar 25, 2014 at 04:06:40PM +0100, Jakub Jelinek wrote:
>> On Tue, Mar 25, 2014 at 12:39:18PM +0100, Uros Bizjak wrote:
>> > The patch is OK in principle, but we could follow established practice
>> > and use separate predicates - please see general_szext_operand mode
>> > attribute definition.
>>
>> So like this?  I've tried to use non-VOIDmode of the predicates that were
>> used previously (i.e. general_operand or x86_64_general_operand).
>>
>> 2014-03-25  Jakub Jelinek  <jakub@redhat.com>
>>
>>       * config/i386/i386.md (general_sext_operand): New mode attr.
>>       (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> and <general_sext_operand>
>>       predicate instead of <general_operand>.
>>       (*addv<mode>4_1, *subv<mode>4_1, *mulv<mode>4_1): New insns.
>>       * config/i386/constraints.md (We): New constraint.
>>       * config/i386/predicates.md (x86_64_sext_operand,
>>       sext_operand): New predicates.
>
> Now successfully bootstrapped/regtested on x86_64-linux and i686-linux.

The patch is OK for mainline.

Thanks,
Uros.
diff mbox

Patch

--- gcc/config/i386/i386.md.jj	2014-03-25 09:22:01.796149575 +0100
+++ gcc/config/i386/i386.md	2014-03-25 11:02:55.194817717 +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,13 @@  (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,7 +5847,8 @@  (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 "x86_64_sext_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")
@@ -5850,6 +5858,31 @@  (define_insn "*addv<mode>4"
   [(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 +6126,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 +6138,13 @@  (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,7 +6152,8 @@  (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 "x86_64_sext_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>")
@@ -6122,6 +6163,31 @@  (define_insn "*subv<mode>4"
   [(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 +6508,97 @@  (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 "x86_64_sext_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)))]
+		   (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]))"
   "@
    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-03-25 09:22:01.848149274 +0100
+++ gcc/config/i386/constraints.md	2014-03-25 10:51:51.190345252 +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
--- gcc/config/i386/predicates.md.jj	2014-03-05 08:28:37.000000000 +0100
+++ gcc/config/i386/predicates.md	2014-03-25 11:00:22.762628436 +0100
@@ -338,6 +338,16 @@  (define_predicate "x86_64_general_operan
 	 (match_operand 0 "x86_64_immediate_operand"))
     (match_operand 0 "general_operand")))
 
+;; Return true if OP is non-VOIDmode general operand representable
+;; on x86_64.  This predicate is used in sign-extending conversion
+;; operations that require non-VOIDmode immediate operands.
+(define_predicate "x86_64_sext_operand"
+  (and (match_test "GET_MODE (op) != VOIDmode")
+       (if_then_else
+	  (match_test "GET_MODE (op) == SImode || GET_MODE (op) == DImode")
+	  (match_operand 0 "x86_64_general_operand")
+	  (match_operand 0 "general_operand"))))
+
 ;; Return true if OP is representable on x86_64 as zero-extended operand.
 ;; This predicate is used in zero-extending conversion operations that
 ;; require non-VOIDmode immediate operands.