[PATCH] i386: Do not modify existing RTL (PR66412)

Uros Bizjak ubizjak@gmail.com
Fri Jun 26 07:44:00 GMT 2015


Hello!

> > > [ I see i386 also does PUT_CODE in a few more splitters, hrm. ]
> >
> > I think the splitters need to be fixed but I also think that
> > shallow_copy_rtx should
> > be enough, no?  combine will not actually end up generating more references
> > to the original RTX so we won't end up with invalid RTX sharing (the same logic
> > is why the splitters didn't care).
>
> That probably is enough; I haven't thought about it too much, all other
> splitters doing a copy do a deep copy.  I don't think it is worth being
> too tricky here, those splitters do not match often at all.
>
> > It would be cleaner to write the splitters in a way that doesnt copy the RTX
> > but creates a new RTX with proper code/mode and operands (but that's more
> > work on your side - maybe Uros wants to help here).
>
> I think people use "copy_rtx; PUT_*" because it is less typing work ;-)
> If that is the only thing you change about the rtx, it even is easier
> to understand than creating a new one.

Attached patch implements both suggestions. It uses shallow_copy_rtx
before all PUT_* calls in the splitters, to avoid nasty surprises like
this one in the future. shallow_ropy_rtx copies just one level of RTX,
and this is all what we need. Also, the patch changes existing uses of
copy_rtx to shallow_copy_rtx.

2015-06-26  Uros Bizjak  <ubizjak@gmail.com>
        Segher Boessenkool  <segher@kernel.crashing.org>

    * config/i386/i386.md (various splitters): Use shallow_copy_rtx
    before doing PUT_MODE or PUT_CODE on operands to avoid
    in-place RTX modification.

testsuite/ChangeLog:

2015-06-26  Uros Bizjak  <ubizjak@gmail.com>

    * gcc.target/i386/pr66412.c: New test.

Patch was bootstrapped and regresison tested on x85_64-linux-gnu
{,-m32}. Patch was committed to mainline SVN and will be backported to
all release branches (once they open).

Uros.
-------------- next part --------------
Index: config/i386/i386.md
===================================================================
--- config/i386/i386.md	(revision 224993)
+++ config/i386/i386.md	(working copy)
@@ -10796,6 +10796,7 @@
   [(set (match_dup 2) (match_dup 1))
    (set (match_dup 0) (zero_extend:DI (match_dup 2)))]
 {
+  operands[1] = shallow_copy_rtx (operands[1]);
   PUT_MODE (operands[1], QImode);
   operands[2] = gen_lowpart (QImode, operands[0]);
 })
@@ -10813,6 +10814,7 @@
    (parallel [(set (match_dup 0) (zero_extend:SI (match_dup 2)))
 	      (clobber (reg:CC FLAGS_REG))])]
 {
+  operands[1] = shallow_copy_rtx (operands[1]);
   PUT_MODE (operands[1], QImode);
   operands[2] = gen_lowpart (QImode, operands[0]);
 })
@@ -10828,6 +10830,7 @@
   [(set (match_dup 2) (match_dup 1))
    (set (match_dup 0) (zero_extend:SI (match_dup 2)))]
 {
+  operands[1] = shallow_copy_rtx (operands[1]);
   PUT_MODE (operands[1], QImode);
   operands[2] = gen_lowpart (QImode, operands[0]);
 })
@@ -10865,7 +10868,10 @@
 	    (const_int 0)))]
   ""
   [(set (match_dup 0) (match_dup 1))]
-  "PUT_MODE (operands[1], QImode);")
+{
+  operands[1] = shallow_copy_rtx (operands[1]);
+  PUT_MODE (operands[1], QImode);
+})
 
 (define_split
   [(set (strict_low_part (match_operand:QI 0 "nonimmediate_operand"))
@@ -10874,7 +10880,10 @@
 	    (const_int 0)))]
   ""
   [(set (match_dup 0) (match_dup 1))]
-  "PUT_MODE (operands[1], QImode);")
+{
+  operands[1] = shallow_copy_rtx (operands[1]);
+  PUT_MODE (operands[1], QImode);
+})
 
 (define_split
   [(set (match_operand:QI 0 "nonimmediate_operand")
@@ -10884,15 +10893,15 @@
   ""
   [(set (match_dup 0) (match_dup 1))]
 {
-  rtx new_op1 = copy_rtx (operands[1]);
-  operands[1] = new_op1;
-  PUT_MODE (new_op1, QImode);
-  PUT_CODE (new_op1, ix86_reverse_condition (GET_CODE (new_op1),
-					     GET_MODE (XEXP (new_op1, 0))));
+  operands[1] = shallow_copy_rtx (operands[1]);
+  PUT_MODE (operands[1], QImode);
+  PUT_CODE (operands[1],
+	    ix86_reverse_condition (GET_CODE (operands[1]),
+				    GET_MODE (XEXP (operands[1], 0))));
 
   /* Make sure that (a) the CCmode we have for the flags is strong
      enough for the reversed compare or (b) we have a valid FP compare.  */
-  if (! ix86_comparison_operator (new_op1, VOIDmode))
+  if (! ix86_comparison_operator (operands[1], VOIDmode))
     FAIL;
 })
 
@@ -10904,15 +10913,15 @@
   ""
   [(set (match_dup 0) (match_dup 1))]
 {
-  rtx new_op1 = copy_rtx (operands[1]);
-  operands[1] = new_op1;
-  PUT_MODE (new_op1, QImode);
-  PUT_CODE (new_op1, ix86_reverse_condition (GET_CODE (new_op1),
-					     GET_MODE (XEXP (new_op1, 0))));
+  operands[1] = shallow_copy_rtx (operands[1]);
+  PUT_MODE (operands[1], QImode);
+  PUT_CODE (operands[1],
+  	    ix86_reverse_condition (GET_CODE (operands[1]),
+				    GET_MODE (XEXP (operands[1], 0))));
 
   /* Make sure that (a) the CCmode we have for the flags is strong
      enough for the reversed compare or (b) we have a valid FP compare.  */
-  if (! ix86_comparison_operator (new_op1, VOIDmode))
+  if (! ix86_comparison_operator (operands[1], VOIDmode))
     FAIL;
 })
 
@@ -11031,7 +11040,10 @@
 	(if_then_else (match_dup 0)
 		      (label_ref (match_dup 1))
 		      (pc)))]
-  "PUT_MODE (operands[0], VOIDmode);")
+{
+  operands[0] = shallow_copy_rtx (operands[0]);
+  PUT_MODE (operands[0], VOIDmode);
+})
 
 (define_split
   [(set (pc)
@@ -11046,15 +11058,15 @@
 		      (label_ref (match_dup 1))
 		      (pc)))]
 {
-  rtx new_op0 = copy_rtx (operands[0]);
-  operands[0] = new_op0;
-  PUT_MODE (new_op0, VOIDmode);
-  PUT_CODE (new_op0, ix86_reverse_condition (GET_CODE (new_op0),
-					     GET_MODE (XEXP (new_op0, 0))));
+  operands[0] = shallow_copy_rtx (operands[0]);
+  PUT_MODE (operands[0], VOIDmode);
+  PUT_CODE (operands[0],
+  	    ix86_reverse_condition (GET_CODE (operands[0]),
+				    GET_MODE (XEXP (operands[0], 0))));
 
   /* Make sure that (a) the CCmode we have for the flags is strong
      enough for the reversed compare or (b) we have a valid FP compare.  */
-  if (! ix86_comparison_operator (new_op0, VOIDmode))
+  if (! ix86_comparison_operator (operands[0], VOIDmode))
     FAIL;
 })
 
@@ -11091,7 +11103,7 @@
 		      (pc)))]
 {
   operands[2] = simplify_gen_subreg (<MODE>mode, operands[2], QImode, 0);
-
+  operands[0] = shallow_copy_rtx (operands[0]);
   PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
 })
 
@@ -11124,7 +11136,7 @@
 		      (pc)))]
 {
   operands[2] = simplify_gen_subreg (<MODE>mode, operands[2], SImode, 0);
-
+  operands[0] = shallow_copy_rtx (operands[0]);
   PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
 })
 
@@ -11160,7 +11172,7 @@
 		      (pc)))]
 {
   operands[2] = simplify_gen_subreg (<MODE>mode, operands[2], SImode, 0);
-
+  operands[0] = shallow_copy_rtx (operands[0]);
   PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
 })
 
@@ -11192,7 +11204,7 @@
 		      (pc)))]
 {
   operands[2] = simplify_gen_subreg (SImode, operands[2], QImode, 0);
-
+  operands[0] = shallow_copy_rtx (operands[0]);
   PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
 })
 
@@ -11228,7 +11240,10 @@
 	(if_then_else (match_op_dup 0 [(reg:CCC FLAGS_REG) (const_int 0)])
 		      (label_ref (match_dup 4))
 		      (pc)))]
-  "PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));")
+{
+  operands[0] = shallow_copy_rtx (operands[0]);
+  PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
+})
 
 ;; Define combination compare-and-branch fp compare instructions to help
 ;; combine.
@@ -17298,6 +17313,7 @@
   operands[1] = gen_lowpart (SImode, operands[1]);
   if (GET_CODE (operands[3]) != ASHIFT)
     operands[2] = gen_lowpart (SImode, operands[2]);
+  operands[3] = shallow_copy_rtx (operands[3]);
   PUT_MODE (operands[3], SImode);
 })
 
Index: testsuite/gcc.target/i386/pr66412.c
===================================================================
--- testsuite/gcc.target/i386/pr66412.c	(revision 0)
+++ testsuite/gcc.target/i386/pr66412.c	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -g" } */
+
+int a, b, c, d;
+
+void
+fn1 ()
+{
+  short e;
+  unsigned short g;
+  
+  for (c = 0; c < 1; c++)
+    d = 0;
+  g = ((a == 0) ^ d) % 8;
+  e = g << 1;
+  b = e && 1;
+}


More information about the Gcc-patches mailing list