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

[Jakub Jelinek <jakub at redhat dot com>]

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