| Message ID | CACUk7=W5x-XVaEPfGvQzuPokN73=Vy9yDXStY0QOq3U10bRPJQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping: Any other opinions on this ? http://gcc.gnu.org/ml/gcc-patches/2012-01/msg00666.html Ramana
> 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.
> 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 --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)); + } }
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