Re: PING^(n+1) fwprop merge (ping 6 or 7 by now)

[Roger Sayle <roger at eyesopen dot com>]

Hi Steven (and Paolo),

On Sun, 7 May 2006, Steven Bosscher wrote:
> >> fwprop merge, stage 2 project:
> >> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg01420.html
> >> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg01421.html
> Ping ping ping ping ping ping!!!!

Constructively, it would help significantly if someone updated these
patches to mainline, rebootstrapped and regression tested them and
resubmitted them.  There have been a significant number of changes
in the last few months, and it seems only reasonable to ensure that
significant changes in stage3 apply cleanly and don't cause regressions.

I'm not sure why Paolo isn't pinging these patches himself, but if
you were to update his contributions, it would show some commitment
that someone is taking ownership for these changes.  Should any
problems occur, its good to know there's someone that can address
technical details in a timely manner.  This is one reason why PINGs
by interested third parties (other than the original contributor)
carry slightly less weight.

These submissions claim to be mostly performance neutral on SPEC, with a
slight hit for PPC, it would be nice if these claims could be supported
with the actual SPEC results.  The primary function of this patch is
compile-time performance benefits of disabling CSE and is mostly clean-up.
Whilst compile-time is a significant issue, it would be nice to see what
the trade-offs are.

Personally, I'd love to see the 3% compile-time improvements from
disabling path-following in CSE provided that the patch is safe and
the performance impact on most platforms is negligible.  However,
there are already complaints about the reasoning used to accept/reject
significant changes in stage3, and whether they are truly ready for
GCC just becuase they are pinged continually.  The fact a patch needs
to be pinged at all often means there's something more the contributor
can do move the review out of the triage twilight zone.

The postings for these patches also reported that these changes had
only been bootstrapped and regression tested on i686-pc-linux-gnu.
If someone was serious about their patches, they'd perhaps do slightly
more than the required minimum testing.  Perhaps two or three primary
platforms?  I also recall that you and I previously had a
"miscommunication", because you'd forgotten to mention that you'd
tested your patch on more platforms than you'd reported.  Maintainers
are not mind readers, so reposting patches after additional testing is
often a good idea.


There's also the issue of breakage and fallout.  Paolo's most recent
patch is still causing mainline regressions on x86_64, see the post
http://gcc.gnu.org/ml/gcc-patches/2006-04/msg00390.html and notice
the "PING^n fwprop" in the title.  I think fixing his own regressions
and other bugzilla PRs should take some priority.  I'm sure I mentioned
previously in stage2/stage3, that fixing a few P1s or any PRs at all,
would allow the same sort of leniency as allowed Richard Guenther, for
example.


A related issue is that part 1 of this patch depended upon a related
fix, http://gcc.gnu.org/ml/gcc-patches/2006-01/msg01824.html to
make sure that 0.0 and 1.0 weren't screwed up by compress_float_constant.
It turns out that this is related to (caused) PR target/27277, for which
Uros has suggested a fix which is still being reviewed.
http://gcc.gnu.org/ml/gcc-patches/2006-05/msg00172.html  I'm not sure
if there any remaining interactions between fwprop and Uros' patch,
but given that there have been fwprop regression in this area in the
past, it only makes sense to check whether they still affect mainline.


Even more constructively, it might help if the df-core.c bug fix from
part 1, was split out and regression tested, resubmitted independently.
One source of confusion, is that parts of these patches have receieved
significant testing, but on the dataflow branch.  Unfortunately, the
significiantly different DF implementation there, poses the question
of how stable these changes are with mainline's DF, beyond the current
testing.



All this is by way of an apology.  Someone should have pointed out
some of these potential issues sooner, but similarly an experienced
contributor might/should have known to address many of these admittedly
non-blocking issues.  You're a maintainer yourself, so I'm sure you
know that your "Ping ping ping ping ping ping!!!!" does little to
pacify any maintainer's doubts and misgivings.



Suggestions:
[1] Ask yourself if the rewards outweigh the risks at the current stage;
    prolonging stage3 whilst ignoring bugs, may actually delay
    improvements longer than returning to stage1 quickly.
[2] Only ping patches multiple times if you're prepared to do more than
    send e-mails, instead ping the original submitter into action.
[3] Avoid pinging patches whilst mainline is unstable or broken.
[4] Split patches into small independent pieces.
[5] Update them against current mainline.
[6] Bootstrap and regression test on two or three platforms.
[7] Repost with supporting benchmark numbers to allow maintainers,
    (and if not clearly a universal win) release managers, to decide.
[8] Fix regressions caused by your changes in a timely manner.
[9] Try to preserve a good track record of not breaking things,
    following the rules, being reasonable and altruistically
    contributing to GCC.

These guidelines apply not only to fwprop, but also autovect,
sign extension elimination, data-flow, improved-aliasing, new
register allocators or any other large functionality fix that needs
to go into the compiler, whilst the maintainers are otherwise
distracted fixing bugs and resolving regressions.

These opinions are purely my own.  The other dozen maintainers who
could also review/approve these patches may have different reasons
for not doing so.  But I suspect that all would agree the above
nine points, would make all our lives easier.

My sincere personal apologies again for the delays.  With luck,
fwprop may be in the tree within a week or so.

Roger
--