Re: [PATCH] Fix force_to_mode not to modify in place the passed rtl (PR rtl-optimization/58726)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Dec 03, 2013 at 09:43:51PM -0700, Jeff Law wrote:
> On 12/03/13 15:46, Jakub Jelinek wrote:
> >As described in the PR, the problem here is that during combine
> >i2 pattern is substituted into more than one place in i3 pattern,
> >unique_copy is 0 (but, even if it would be non-zero, it could happen
> >if the comparison was processed first before the normal set inside of the
> >parallel) and thus the same RTL is (temporarily) shared between two
> >locations.  force_to_mode is first called with mask 0xdc36 (that is
> >actually find for both occurrences in the andhi_2 pattern) and later on
> >inside of the comparison again with mask 0x8000, and as it modifies
> >the IF_THEN_ELSE in place, it modifies also the other location (it is fine
> >if the comparison uses 0x8000 mask, but not in the other spot).
> >As in the end we fold it to a constant, we don't undo it and use incorrect
> >constant.
> >
> >Fixed by making sure force_to_mode doesn't modify x in place.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
> >
> >2013-12-03  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR rtl-optimization/58726
> >	* combine.c (force_to_mode): Fix comment typo.  Don't destructively
> >	modify x for ROTATE, ROTATERT and IF_THEN_ELSE.
> >
> >	* gcc.c-torture/execute/pr58726.c: New test.
> I'd worry there's other latent bugs of this nature and if we'd be
> better off avoiding the temporary sharing.  We have structure
> sharing rules for a reason -- I'd hate to think of all the code that
> would need auditing to ensure it was safe with this bogus sharing.

I'm afraid I'm not familiar with the unwritten rules of combine.c enough
to know whether the above fix is all we need, or if there are further issues
just latent.

> Any thoughts of how painful it'd be to avoid the sharing to start with?

Perhaps most of the subst function could be moved to subst_1, drop the
and subst as a wrapper (with dropped unique_copy argument?) could do
for_each_rtx first to find out how many occurrences of from there are
in x, and if it is more than one, subst_1 then would copy_rtx for each
return of to other than the last one.  IMHO doing copy_rtx unconditionally
would be too expensive for the common case, would create too much garbage.

But it doesn't look like a safe thing to do on the release branches, at
least not until it is for a few months on the trunk.

	Jakub