diff mbox

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

Message ID CACUk7=W5x-XVaEPfGvQzuPokN73=Vy9yDXStY0QOq3U10bRPJQ@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Ramana Radhakrishnan Jan. 13, 2012, 10:31 a.m. UTC
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

Comments

Ramana Radhakrishnan Jan. 20, 2012, 9:32 a.m. UTC | #1
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. UTC | #2
> 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. UTC | #3
> 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.
diff mbox

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));
+
        }
     }