[PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)

[PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)

[Jakub Jelinek]

Hi!

The force_operand on complex count expression in doloop_modify can invoke
various expander routines that are assuming there is rtl unsharing after
them (which is the case for expansion).  When later optimizations invoke
the expander (e.g. expand_mult in this case), they use
unshare_all_rtl_in_chain on the sequence.  The following patch does that for
doloop.  The count expression is already known not to be shared with
anything else (we do copy_rtx on it first and then create new rtls around it
if needed), so for that if it occurs just once in the sequence, we don't
need to unshare it.  For subexpression of condition I'm not sure, which is
why I've forced unsharing even if it occurs just once and is not shareable
part of the condition like REG.

Bootstrapped/regtested on powerpc64-linux, ok for trunk?

2017-01-14  Jakub Jelinek  <jakub@redhat.com>

	PR target/79080
	* loop-doloop.c (doloop_modify): Call unshare_all_rtl_in_chain on
	sequence.  Formatting fixes.
	(doloop_optimize): Formatting fixes.

	* gcc.dg/pr79080.c: New test.

--- gcc/loop-doloop.c.jj	2017-01-01 12:45:37.000000000 +0100
+++ gcc/loop-doloop.c	2017-01-13 09:55:36.918702356 +0100
@@ -479,9 +479,13 @@ doloop_modify (struct loop *loop, struct
 
   /* Insert initialization of the count register into the loop header.  */
   start_sequence ();
+  /* count has been already copied through copy_rtx.  */
+  reset_used_flags (count);
+  set_used_flags (condition);
   tmp = force_operand (count, counter_reg);
   convert_move (counter_reg, tmp, 1);
   sequence = get_insns ();
+  unshare_all_rtl_in_chain (sequence);
   end_sequence ();
   emit_insn_after (sequence, BB_END (loop_preheader_edge (loop)->src));
 
@@ -489,10 +493,8 @@ doloop_modify (struct loop *loop, struct
     {
       rtx ass = copy_rtx (desc->noloop_assumptions);
       basic_block preheader = loop_preheader_edge (loop)->src;
-      basic_block set_zero
-	      = split_edge (loop_preheader_edge (loop));
-      basic_block new_preheader
-	      = split_edge (loop_preheader_edge (loop));
+      basic_block set_zero = split_edge (loop_preheader_edge (loop));
+      basic_block new_preheader = split_edge (loop_preheader_edge (loop));
       edge te;
 
       /* Expand the condition testing the assumptions and if it does not pass,
@@ -688,8 +690,7 @@ doloop_optimize (struct loop *loop)
   rtx_insn *doloop_seq = targetm.gen_doloop_end (doloop_reg, start_label);
 
   word_mode_size = GET_MODE_PRECISION (word_mode);
-  word_mode_max
-	  = (HOST_WIDE_INT_1U << (word_mode_size - 1) << 1) - 1;
+  word_mode_max = (HOST_WIDE_INT_1U << (word_mode_size - 1) << 1) - 1;
   if (! doloop_seq
       && mode != word_mode
       /* Before trying mode different from the one in that # of iterations is
--- gcc/testsuite/gcc.dg/pr79080.c.jj	2017-01-13 10:03:54.518423577 +0100
+++ gcc/testsuite/gcc.dg/pr79080.c	2017-01-13 10:07:37.610608570 +0100
@@ -0,0 +1,19 @@
+/* PR target/79080 */
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+/* { dg-additional-options "-mcpu=8548" { target { powerpc*-*-* && ilp32 } } } */
+
+int
+foo (char x)
+{
+  int a;
+
+  for (;;)
+    {
+      x += 59;
+      if (x != 0)
+        a = 0;
+      else
+        return 0;
+    }
+}

	Jakub

Re: [PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)

[Jeff Law]

On 01/14/2017 06:36 AM, Jakub Jelinek wrote:
> Hi!
>
> The force_operand on complex count expression in doloop_modify can invoke
> various expander routines that are assuming there is rtl unsharing after
> them (which is the case for expansion).  When later optimizations invoke
> the expander (e.g. expand_mult in this case), they use
> unshare_all_rtl_in_chain on the sequence.  The following patch does that for
> doloop.  The count expression is already known not to be shared with
> anything else (we do copy_rtx on it first and then create new rtls around it
> if needed), so for that if it occurs just once in the sequence, we don't
> need to unshare it.  For subexpression of condition I'm not sure, which is
> why I've forced unsharing even if it occurs just once and is not shareable
> part of the condition like REG.
>
> Bootstrapped/regtested on powerpc64-linux, ok for trunk?
>
> 2017-01-14  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/79080
> 	* loop-doloop.c (doloop_modify): Call unshare_all_rtl_in_chain on
> 	sequence.  Formatting fixes.
> 	(doloop_optimize): Formatting fixes.
>
> 	* gcc.dg/pr79080.c: New test.
So do we have a more general problem here -- we use force_operand in all 
kinds of contexts, particularly in the RTL loop optimizers.  If 
force_operand can introduce undesirable sharing, then isn't more code 
prone to this problem?

So just to be clear, I'm OK with this change as-is, I just worry we have 
other places that are structure sharing assumption problems waiting to 
happen.

jeff

Re: [PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)

[Jakub Jelinek]

On Sat, Jan 14, 2017 at 09:16:11PM -0700, Jeff Law wrote:
> > The force_operand on complex count expression in doloop_modify can invoke
> > various expander routines that are assuming there is rtl unsharing after
> > them (which is the case for expansion).  When later optimizations invoke
> > the expander (e.g. expand_mult in this case), they use
> > unshare_all_rtl_in_chain on the sequence.  The following patch does that for
> > doloop.  The count expression is already known not to be shared with
> > anything else (we do copy_rtx on it first and then create new rtls around it
> > if needed), so for that if it occurs just once in the sequence, we don't
> > need to unshare it.  For subexpression of condition I'm not sure, which is
> > why I've forced unsharing even if it occurs just once and is not shareable
> > part of the condition like REG.
> > 
> > Bootstrapped/regtested on powerpc64-linux, ok for trunk?
> > 
> > 2017-01-14  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/79080
> > 	* loop-doloop.c (doloop_modify): Call unshare_all_rtl_in_chain on
> > 	sequence.  Formatting fixes.
> > 	(doloop_optimize): Formatting fixes.
> > 
> > 	* gcc.dg/pr79080.c: New test.
> So do we have a more general problem here -- we use force_operand in all
> kinds of contexts, particularly in the RTL loop optimizers.  If
> force_operand can introduce undesirable sharing, then isn't more code prone
> to this problem?

I think it is not as bad.  I think the problem is only when you
force_operand 1) after expansion 2) with complicated expression.
I think if you force_operand just with something that appears in some insn,
it is very unlikely it will create something significantly more complex.
It is just the case where something constructs a complex RTX expression and
then produces insns using the force_operand.  In the doloop case, it is
desc->niter_expr expression, where possibly several insns in the IL together
are used to compute the niter_expr.  Or another case could be when using
content of REG_EQUAL note and trying to force_operand it.

> So just to be clear, I'm OK with this change as-is, I just worry we have
> other places that are structure sharing assumption problems waiting to
> happen.

We have rtl sharing verification, while it has been defunct for about 3
years, it is in effect again for some time already and not everything in the
RTL are changed during the last 3 years, before that it had to pass that
verification too.  I think it is fine to just wait if similar RTL sharing
issues are reported next time, it is hard to guess where it would be
actually needed and where it would be just a waste of time.

	Jakub

Re: [PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)

[Richard Biener]

On Sun, 15 Jan 2017, Jakub Jelinek wrote:

> On Sat, Jan 14, 2017 at 09:16:11PM -0700, Jeff Law wrote:
> > > The force_operand on complex count expression in doloop_modify can invoke
> > > various expander routines that are assuming there is rtl unsharing after
> > > them (which is the case for expansion).  When later optimizations invoke
> > > the expander (e.g. expand_mult in this case), they use
> > > unshare_all_rtl_in_chain on the sequence.  The following patch does that for
> > > doloop.  The count expression is already known not to be shared with
> > > anything else (we do copy_rtx on it first and then create new rtls around it
> > > if needed), so for that if it occurs just once in the sequence, we don't
> > > need to unshare it.  For subexpression of condition I'm not sure, which is
> > > why I've forced unsharing even if it occurs just once and is not shareable
> > > part of the condition like REG.
> > > 
> > > Bootstrapped/regtested on powerpc64-linux, ok for trunk?
> > > 
> > > 2017-01-14  Jakub Jelinek  <jakub@redhat.com>
> > > 
> > > 	PR target/79080
> > > 	* loop-doloop.c (doloop_modify): Call unshare_all_rtl_in_chain on
> > > 	sequence.  Formatting fixes.
> > > 	(doloop_optimize): Formatting fixes.
> > > 
> > > 	* gcc.dg/pr79080.c: New test.
> > So do we have a more general problem here -- we use force_operand in all
> > kinds of contexts, particularly in the RTL loop optimizers.  If
> > force_operand can introduce undesirable sharing, then isn't more code prone
> > to this problem?
> 
> I think it is not as bad.  I think the problem is only when you
> force_operand 1) after expansion 2) with complicated expression.
> I think if you force_operand just with something that appears in some insn,
> it is very unlikely it will create something significantly more complex.
> It is just the case where something constructs a complex RTX expression and
> then produces insns using the force_operand.  In the doloop case, it is
> desc->niter_expr expression, where possibly several insns in the IL together
> are used to compute the niter_expr.  Or another case could be when using
> content of REG_EQUAL note and trying to force_operand it.

Maybe we want to have a force_operand_and_unshare () function then?

Richard.

> > So just to be clear, I'm OK with this change as-is, I just worry we have
> > other places that are structure sharing assumption problems waiting to
> > happen.
> 
> We have rtl sharing verification, while it has been defunct for about 3
> years, it is in effect again for some time already and not everything in the
> RTL are changed during the last 3 years, before that it had to pass that
> verification too.  I think it is fine to just wait if similar RTL sharing
> issues are reported next time, it is hard to guess where it would be
> actually needed and where it would be just a waste of time.
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)

[Jakub Jelinek]

On Mon, Jan 16, 2017 at 08:23:32AM +0100, Richard Biener wrote:
> > I think it is not as bad.  I think the problem is only when you
> > force_operand 1) after expansion 2) with complicated expression.
> > I think if you force_operand just with something that appears in some insn,
> > it is very unlikely it will create something significantly more complex.
> > It is just the case where something constructs a complex RTX expression and
> > then produces insns using the force_operand.  In the doloop case, it is
> > desc->niter_expr expression, where possibly several insns in the IL together
> > are used to compute the niter_expr.  Or another case could be when using
> > content of REG_EQUAL note and trying to force_operand it.
> 
> Maybe we want to have a force_operand_and_unshare () function then?

Not sure.  1) grepped around and this has been the only place that calls
force_operand where the unsharing is known to be needed (perhaps so far).
Other places use expand_simple_* (multiple), etc. directly 2) for the unsharing,
it is good to know if the expression(s) you are forcing appear or may appear
somewhere else in the IL or not.  Such information wouldn't be known by
force_operand_and_unshare, so it would always have to unshare everything,
even if it occurs just once in the sequence.

	Jakub