Re: [PATCH] Avoid optimizing memory references with side effects in compare-elim.c

[Paul Koning <paulkoning at comcast dot net>]

> 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;
+}