This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)
- From: Richard Biener <rguenther at suse dot de>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Jeff Law <law at redhat dot com>, Eric Botcazou <ebotcazou at adacore dot com>, gcc-patches at gcc dot gnu dot org
- Date: Mon, 16 Jan 2017 08:23:32 +0100 (CET)
- Subject: Re: [PATCH] Fix RTL sharing bug in loop-doloop.c (PR target/79080)
- Authentication-results: sourceware.org; auth=none
- References: <20170114133634.GE1867@tucnak> <c6c02bd6-720a-750a-7ea6-e8624a652ad3@redhat.com> <20170115191229.GI1867@tucnak>
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)