Patchwork [RFC,combine] PR48308 - Fix issue with missing(?) LOG_LINKS

login
register
mail settings
Submitter Ramana Radhakrishnan
Date Jan. 13, 2012, 10:31 a.m.
Message ID <CACUk7=W5x-XVaEPfGvQzuPokN73=Vy9yDXStY0QOq3U10bRPJQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/135777/
State New
Headers show

Comments

Ramana Radhakrishnan - Jan. 13, 2012, 10:31 a.m.
Hi,

PR48308 is a case where on the ARM port we incorrectly delete a call
to strcmp assuming that this is dead. However the problem starts early
enough in combine where when try_combine is given i2 of the form

   (parallel [(set (reg:CC X) (compare:CC OP (const_int 0)))
                   (set Y OP)])

and i1 is NULL

it transforms this to :


    (set Y OP)

  and change I2 to be

        (set (reg:CC X) (compare:CC Y (const_int 0)))

but in the snippet of code that changes i1 and i2 we don't seem to
update LOG_LINKS .

We then go and check if i1_feeds_into_i2 and that check relies on the
LOG_LINKS being up-to-date. We find that i2 can be combined with i3
*but* we've then gone and made a transformation that results in Y
being used but miss out emitting the set of Y.

The attached patch appears to fix the problem for the reduced testcase
and reduced^2 testcase attached to PR48308.

Unfortunately this doesn't fix the testcase from PR50313 which
prima-facie was a dup of this bug which I'm still investigating. This
has survived bootstrap on x86_64 and is running tests there ( though
based on a quick reading of the x86 backend I couldn't find any
parallels of that form) and is still undergoing a full bootstrap run
on an ARM board.

Thoughts ?


cheers
Ramana

 #endif
Ramana Radhakrishnan - Jan. 20, 2012, 9:32 a.m.
Ping:

Any other opinions on this ?

http://gcc.gnu.org/ml/gcc-patches/2012-01/msg00666.html


Ramana
Eric Botcazou - Jan. 21, 2012, 5:16 p.m.
> We then go and check if i1_feeds_into_i2 and that check relies on the
> LOG_LINKS being up-to-date. We find that i2 can be combined with i3
> *but* we've then gone and made a transformation that results in Y
> being used but miss out emitting the set of Y.
>
> The attached patch appears to fix the problem for the reduced testcase
> and reduced^2 testcase attached to PR48308.
>
> Unfortunately this doesn't fix the testcase from PR50313 which
> prima-facie was a dup of this bug which I'm still investigating. This
> has survived bootstrap on x86_64 and is running tests there ( though
> based on a quick reading of the x86 backend I couldn't find any
> parallels of that form) and is still undergoing a full bootstrap run
> on an ARM board.
>
> Thoughts ?

The un-combining thing around line 2800 is somewhat of a kludge and I wouldn't 
be surprised if there were other fallouts.  But your change is clearly correct 
and looks relatively safe, so OK for trunk and 4.6 branch after full testing.
Eric Botcazou - Jan. 22, 2012, 10:53 a.m.
> The un-combining thing around line 2800 is somewhat of a kludge and I
> wouldn't be surprised if there were other fallouts.  But your change is
> clearly correct and looks relatively safe, so OK for trunk and 4.6 branch
> after full testing.

I overlooked something though: it might be possible for combine_instructions to 
try to combine i2 again if the previous combination fails (if it succeeds, i1 
is deleted so this is OK) so the stall LOG_LINKS could be problematic.  That's 
why LOG_LINKS (i2) needs to SUBST-ituted like the two lines just above.

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index 4178870..f6b8769 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2865,6 +2865,8 @@  try_combine (rtx i3, rtx i2, rtx i1, rtx i0, int
*new_direct_jump_p,
          SUBST (PATTERN (i2), XVECEXP (PATTERN (i2), 0, 0));
          SUBST (XEXP (SET_SRC (PATTERN (i2)), 0),
                 SET_DEST (PATTERN (i1)));
+         LOG_LINKS (i2) = alloc_insn_link (i1, LOG_LINKS (i2));
+
        }
     }