Avoid optimizing memory references with side effects in compare-elim.c

Message ID 03256FC2-7A81-44D7-B595-A9978D0E529C@comcast.net
State Superseded
Headers show
Series
  • Avoid optimizing memory references with side effects in compare-elim.c
Related show

Commit Message

Paul Koning June 5, 2018, 5:11 p.m.
This fixes an ICE if post-reload compare elimination is done and the target supports post_inc and similar modes, as pdp11 does.  The ICE is caused by a generated PARALLEL that has the side effect twice, which is not legal.

(Ideally it would generate a similar RTL suitable for a matching constraint with the side effect omitted; I may try for that later on if that is still supported by the constraint machinery.)

Tested against my in-progress CCmode pdp11 target.  Ok to commit?

	paul

gcc/ChangeLog:

2018-06-05  Paul Koning  <ni1d@arrl.net>

	* compare-elim.c (addr_side_effect_check): New function.
	(addr_side_effect_p): Ditto.
	(try_merge_compare): Don't merge compare if address contains a
	side effect.

Comments

Richard Sandiford June 5, 2018, 7:54 p.m. | #1
Paul Koning <paulkoning@comcast.net> writes:
> This fixes an ICE if post-reload compare elimination is done and the

> target supports post_inc and similar modes, as pdp11 does.  The ICE is

> caused by a generated PARALLEL that has the side effect twice, which

> is not legal.

>

> (Ideally it would generate a similar RTL suitable for a matching

> constraint with the side effect omitted; I may try for that later on

> if that is still supported by the constraint machinery.)

>

> Tested against my in-progress CCmode pdp11 target.  Ok to commit?


Thanks for doing the conversion :-)

> Index: compare-elim.c

> ===================================================================

> --- compare-elim.c	(revision 261207)

> +++ compare-elim.c	(working copy)

> @@ -127,6 +127,23 @@

>    

>  static vec<comparison *> all_compares;

>  

> +/* Callback function used by addr_side_effect_p.  */

> +static int

> +addr_side_effect_check (rtx mem ATTRIBUTE_UNUSED, rtx op ATTRIBUTE_UNUSED,

> +			rtx dest ATTRIBUTE_UNUSED, rtx src ATTRIBUTE_UNUSED,

> +			rtx srcoff ATTRIBUTE_UNUSED, void *arg ATTRIBUTE_UNUSED)

> +{

> +  return 1;

> +}

> +

> +/* Check if addr has side effects (contains autoinc or autodec

> +   operations).  */

> +static int

> +addr_side_effect_p (rtx addr)

> +{

> +  return for_each_inc_dec (addr, addr_side_effect_check, NULL);

> +} 

> +  

>  /* Look for a "conforming" comparison, as defined above.  If valid, return

>     the rtx for the COMPARE itself.  */

>  

> @@ -690,6 +707,13 @@

>      return false;

>  

>    rtx src = SET_SRC (set);

> +

> +  /* If the source uses addressing modes with side effects, we can't

> +     do the merge because we'd end up with a PARALLEL that has two

> +     instances of that side effect in it.  */

> +  if (addr_side_effect_p (src))

> +    return false;

> +

>    rtx flags = maybe_select_cc_mode (cmp, src, CONST0_RTX (GET_MODE (src)));

>    if (!flags)

>      {

> @@ -809,6 +833,12 @@

>    else

>      return false;

>  

> +  /* If the source uses addressing modes with side effects, we can't

> +     do the merge because we'd end up with a PARALLEL that has two

> +     instances of that side effect in it.  */

> +  if (addr_side_effect_p (cmp_src))

> +    return false;

> +

>    /* Determine if we ought to use a different CC_MODE here.  */

>    flags = maybe_select_cc_mode (cmp, cmp_src, in_b);

>    if (flags == NULL)


It looks like the more general side_effects_p might be better
in both cases.  It's simpler than defining a custom function,
and I'm not convinced the pass would handle other kinds of
side-effect correctly either.

FWIW, I agree these look like good places to put the check.
They're early enough to bail out, but late enough not to slow
down the common case.

Thanks,
Richard
Paul Koning June 5, 2018, 9:29 p.m. | #2
> On Jun 5, 2018, at 3:54 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote:

> 

> Paul Koning <paulkoning@comcast.net> writes:

>> This fixes an ICE if post-reload compare elimination is done and the

>> target supports post_inc and similar modes, as pdp11 does.  The ICE is

>> caused by a generated PARALLEL that has the side effect twice, which

>> is not legal.

>> 

>> (Ideally it would generate a similar RTL suitable for a matching

>> constraint with the side effect omitted; I may try for that later on

>> if that is still supported by the constraint machinery.)

>> 

>> Tested against my in-progress CCmode pdp11 target.  Ok to commit?

> 

> Thanks for doing the conversion :-)


It has a ways to go still.  And more changes to compare-elim may be needed, since pdp11 is an oddball that has two separate CC registers (one for the CPU, one for the FP coprocessor).  Compare-elim doesn't currently handle that.

Credit to Eric Botcazou, whose documentation makes this a whole lot easier.

> ...

> It looks like the more general side_effects_p might be better

> in both cases.  It's simpler than defining a custom function,

> and I'm not convinced the pass would handle other kinds of

> side-effect correctly either.


Thanks, I overlooked that function.  Yes, it's a better choice.  For example, compare-eliminating a volatile memory reference seems incorrect (it's not clear how likely it is to happen, but it makes sense to skip that case).

That simplifies the patch, which now looks like this.  Ok for trunk?

	paul

gcc/ChangeLog:

2018-06-05  Paul Koning  <ni1d@arrl.net>

	* compare-elim.c (try_merge_compare): Don't merge compare if
	address contains a side effect.
	* gcc.c-torture/compile/20180605-1.c: New test case.

Index: compare-elim.c
===================================================================
--- compare-elim.c	(revision 261207)
+++ compare-elim.c	(working copy)
@@ -690,6 +690,13 @@
     return false;
 
   rtx src = SET_SRC (set);
+
+  /* If the source uses addressing modes with side effects, we can't
+     do the merge because we'd end up with a PARALLEL that has two
+     instances of that side effect in it.  */
+  if (side_effects_p (src))
+    return false;
+
   rtx flags = maybe_select_cc_mode (cmp, src, CONST0_RTX (GET_MODE (src)));
   if (!flags)
     {
@@ -809,6 +816,12 @@
   else
     return false;
 
+  /* If the source uses addressing modes with side effects, we can't
+     do the merge because we'd end up with a PARALLEL that has two
+     instances of that side effect in it.  */
+  if (side_effects_p (cmp_src))
+    return false;
+
   /* Determine if we ought to use a different CC_MODE here.  */
   flags = maybe_select_cc_mode (cmp, cmp_src, in_b);
   if (flags == NULL)
Index: testsuite/gcc.c-torture/compile/20180605-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/20180605-1.c	(nonexistent)
+++ testsuite/gcc.c-torture/compile/20180605-1.c	(working copy)
@@ -0,0 +1,9 @@
+void f (int *p, int n)
+{
+    int j = 0, k;
+    
+    for (int i = 0; i < n; i++)
+        if ((k = *p++) > 0)
+            j += k;
+    return j;
+}
Eric Botcazou June 6, 2018, 3:53 p.m. | #3
> That simplifies the patch, which now looks like this.  Ok for trunk?

> 

> 	paul

> 

> gcc/ChangeLog:

> 

> 2018-06-05  Paul Koning  <ni1d@arrl.net>

> 

> 	* compare-elim.c (try_merge_compare): Don't merge compare if

> 	address contains a side effect.

> 	* gcc.c-torture/compile/20180605-1.c: New test case.


The patch is OK, but its ChangeLog is not. :-)  It does not modify one 
function but two (try_merge_compare and try_eliminate_compare).  I would 
suggest using "diff -up" to generate patches with more context.

-- 
Eric Botcazou
Paul Koning June 6, 2018, 6:23 p.m. | #4
> On Jun 6, 2018, at 11:53 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:

> 

>> That simplifies the patch, which now looks like this.  Ok for trunk?

>> 

>> 	paul

>> 

>> gcc/ChangeLog:

>> 

>> 2018-06-05  Paul Koning  <ni1d@arrl.net>

>> 

>> 	* compare-elim.c (try_merge_compare): Don't merge compare if

>> 	address contains a side effect.

>> 	* gcc.c-torture/compile/20180605-1.c: New test case.

> 

> The patch is OK, but its ChangeLog is not. :-)  It does not modify one 

> function but two (try_merge_compare and try_eliminate_compare).  I would 

> suggest using "diff -up" to generate patches with more context.


Yes, sorry.  I normally use the subversion internal diff which doesn't do this.

Here is a corrected version.  Ok with this change?

	paul

gcc/ChangeLog:

2018-06-06  Paul Koning  <ni1d@arrl.net>

	* compare-elim.c (try_merge_compare): Don't merge compare if
	address contains a side effect.
	(try_eliminate_compare): Likewise.
	* gcc.c-torture/compile/20180605-1.c: New test case.

Index: compare-elim.c
===================================================================
--- compare-elim.c	(revision 261207)
+++ compare-elim.c	(working copy)
@@ -690,6 +690,13 @@ try_merge_compare (struct comparison *cm
     return false;
 
   rtx src = SET_SRC (set);
+
+  /* If the source uses addressing modes with side effects, we can't
+     do the merge because we'd end up with a PARALLEL that has two
+     instances of that side effect in it.  */
+  if (side_effects_p (src))
+    return false;
+
   rtx flags = maybe_select_cc_mode (cmp, src, CONST0_RTX (GET_MODE (src)));
   if (!flags)
     {
@@ -809,6 +816,12 @@ try_eliminate_compare (struct comparison
   else
     return false;
 
+  /* If the source uses addressing modes with side effects, we can't
+     do the merge because we'd end up with a PARALLEL that has two
+     instances of that side effect in it.  */
+  if (side_effects_p (cmp_src))
+    return false;
+
   /* Determine if we ought to use a different CC_MODE here.  */
   flags = maybe_select_cc_mode (cmp, cmp_src, in_b);
   if (flags == NULL)
Index: testsuite/gcc.c-torture/compile/20180605-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/20180605-1.c	(nonexistent)
+++ testsuite/gcc.c-torture/compile/20180605-1.c	(working copy)
@@ -0,0 +1,9 @@
+void f (int *p, int n)
+{
+    int j = 0, k;
+    
+    for (int i = 0; i < n; i++)
+        if ((k = *p++) > 0)
+            j += k;
+    return j;
+}
Peter Bergner June 6, 2018, 6:35 p.m. | #5
On 6/6/18 1:23 PM, Paul Koning wrote:
>> On Jun 6, 2018, at 11:53 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:

>> I would suggest using "diff -up" to generate patches with more context.

> 

> Yes, sorry.  I normally use the subversion internal diff which doesn't do this.


If you edit ~/.subversion/config and use the line:

  diff-extensions = -u -p

that will do that for you.  It's probably already in that config file and
just needs to be uncommented out.

Peter
Eric Botcazou June 7, 2018, 3:37 p.m. | #6
> Here is a corrected version.  Ok with this change?

> 

> 	paul

> 

> gcc/ChangeLog:

> 

> 2018-06-06  Paul Koning  <ni1d@arrl.net>

> 

> 	* compare-elim.c (try_merge_compare): Don't merge compare if

> 	address contains a side effect.

> 	(try_eliminate_compare): Likewise.

> 	* gcc.c-torture/compile/20180605-1.c: New test case.


OK for mainline, thanks.

-- 
Eric Botcazou
Paul Koning June 7, 2018, 5:57 p.m. | #7
> On Jun 7, 2018, at 11:37 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:

> 

>> Here is a corrected version.  Ok with this change?

>> 

>> 	paul

>> 

>> gcc/ChangeLog:

>> 

>> 2018-06-06  Paul Koning  <ni1d@arrl.net>

>> 

>> 	* compare-elim.c (try_merge_compare): Don't merge compare if

>> 	address contains a side effect.

>> 	(try_eliminate_compare): Likewise.

>> 	* gcc.c-torture/compile/20180605-1.c: New test case.

> 

> OK for mainline, thanks.


Thanks.  I just realized that the testsuite has its own ChangeLog.  
So committed with the change message split between those two.

	paul


gcc/ChangeLog:

2018-06-07  Paul Koning  <ni1d@arrl.net>

	* compare-elim.c (try_merge_compare): Don't merge compare if
	address contains a side effect.
	(try_eliminate_compare): Likewise.

gcc/testsuite/ChangeLog:

2018-06-07  Paul Koning  <ni1d@arrl.net>

	* gcc.c-torture/compile/20180605-1.c: New test.

Patch

Index: compare-elim.c
===================================================================
--- compare-elim.c	(revision 261207)
+++ compare-elim.c	(working copy)
@@ -127,6 +127,23 @@ 
   
 static vec<comparison *> all_compares;
 
+/* Callback function used by addr_side_effect_p.  */
+static int
+addr_side_effect_check (rtx mem ATTRIBUTE_UNUSED, rtx op ATTRIBUTE_UNUSED,
+			rtx dest ATTRIBUTE_UNUSED, rtx src ATTRIBUTE_UNUSED,
+			rtx srcoff ATTRIBUTE_UNUSED, void *arg ATTRIBUTE_UNUSED)
+{
+  return 1;
+}
+
+/* Check if addr has side effects (contains autoinc or autodec
+   operations).  */
+static int
+addr_side_effect_p (rtx addr)
+{
+  return for_each_inc_dec (addr, addr_side_effect_check, NULL);
+} 
+  
 /* Look for a "conforming" comparison, as defined above.  If valid, return
    the rtx for the COMPARE itself.  */
 
@@ -690,6 +707,13 @@ 
     return false;
 
   rtx src = SET_SRC (set);
+
+  /* If the source uses addressing modes with side effects, we can't
+     do the merge because we'd end up with a PARALLEL that has two
+     instances of that side effect in it.  */
+  if (addr_side_effect_p (src))
+    return false;
+
   rtx flags = maybe_select_cc_mode (cmp, src, CONST0_RTX (GET_MODE (src)));
   if (!flags)
     {
@@ -809,6 +833,12 @@ 
   else
     return false;
 
+  /* If the source uses addressing modes with side effects, we can't
+     do the merge because we'd end up with a PARALLEL that has two
+     instances of that side effect in it.  */
+  if (addr_side_effect_p (cmp_src))
+    return false;
+
   /* Determine if we ought to use a different CC_MODE here.  */
   flags = maybe_select_cc_mode (cmp, cmp_src, in_b);
   if (flags == NULL)