This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]