Re: Your proposed change to loop.c

[Mark Mitchell <mark at codesourcery dot com>]

>>>>> "Joern" == Joern Rennecke <amylaar@cygnus.co.uk> writes:

    Joern> Then we'd pretend to have data that we don't have.  The
    Joern> register is either live after the loop or not, but we don't
    Joern> know which when we record the giv.  Computing the value
    Joern> costs pretty much the same for a single giv as for all
    Joern> givs, so it is done for all givs simultanously.

I understand what you're saying, but I think we disagree on an issue
of style.  By not initializing the field, we're "pretending" to know
the answer anyhow; some value will end up in the field.  I'm thinking
of someone trying to fix a bug later on down the road, wondering why
that field has the value it has, looking at record_giv, guessing "aha,
I bet that's uninitialized", and going off on a wild goose chase.

As long as you're sure this gets initialized, I'm OK with it.  But, a
comment about when the field is set might be reasonable, yes?

    >> Now, the hard questions:
    >> 
    >> o I don't understand what the bug is in find_life_end that is
    >> causing the current endless loop.  Please explain it to me,
    >> bearing in mind that I am not an long-time expert loop.c hacker
    >> like yourself!  (Of course, I do know about compilers in
    >> general, and GCC in particular, and understand loop
    >> optimizations.)  I don't find the loop.c comments terribly
    >> helpful (not the ones in your patch, the ones already in the
    >> code.)
    >> 
    >> I'd also like to know on which system your original change was
    >> bootstrapped, and why the bug didn't appear there.  That would
    >> probably help me understand what's going on a little better.

    Joern> It was bootstrapped on i686-pc-linux-gnulibc1 .  Note that
    Joern> patches are seldom approved in the same order nor the same
    Joern> context as they were submitted and tested before submittal.

Unfortunately, this answer didn't really answer my questions.  In
particular, I still don't understand the bug.  Why are we getting
stuck in an endless loop?  What about your previous changes caused
this problem?  If you just unconvered a latent bug, what is the bug,
and how did it get uncovered?

I don't really agree with your comments about the order in which
patches are installed.  Perhaps our experience differs. 

 In any case, wouldn't you agree that you have a responsibility to
test each check-in?  In other words, before you check in a patch,
apply just that patch to an otherwise virgin tree and test?  I can't
tell from your comments whether you did this, or whether you tested a
much larger patch sometime back, and just recently applied a little
piece of it.

    >> o If the above has not yet made it obvious, please explain why
    >> there is not a simpler fix.  Is there a bug in find_life_end?
    >> If not, is the code that uses it not treating its return value
    >> correctly?  It looks to me like the point of live_after_loop is
    >> to enable moving instructions around.  You removed this code:
    >> 
    >> - /* Check that the giv insn we're about to use for deriving -
    >> precedes all uses of that giv.  Note that initializing the -
    >> derived giv would defeat the purpose of reducing register -
    >> pressure.  - ??? We could arrange to move the insn.  */ - &&
    >> ((unsigned) stats[i].end_luid - INSN_LUID (loop_start) - >
    >> (unsigned) stats[i].start_luid - INSN_LUID (loop_start)
    >> 
    >> That suggests to me that perhaps you're doing some additional
    >> optimization that we were not doing before?  Can we not do this
    >> extra optimization, but still fix the bug?

    Joern> I've actually done the instruction motion first and then
    Joern> later found out that find_life_end was bogus.  I can't
    Joern> remember what the probles actually were, it was 3 month ago
    Joern> that I did the rewrite.  I've removed the moving of the biv
    Joern> insn from the patch for a simpler bug fix already.  For
    Joern> about 30% extra you'd get derivation of givs from
    Joern> non-eliminable bivs, DEST_ADDR giv derivation and biv insn
    Joern> movement.

I'd love to have all the great optimizations you're working on.  But,
first I need to understand the current patch.  What does it do?  Why?

    >> o If there's no simpler fix, why does this fix the bug?  What's
    >> the top-level summary of your approach?  Did you bootstrap/test
    >> this change on one of the systems affected by the current
    >> problem?

    Joern> You might be able to find a simpler fix.  But it wouldn't
    Joern> help long-term since the redesign is also needed for the
    Joern> new optimizations.  I gauge the lifetime of givs by the
    Joern> span from their first to last use in the loop.  Yes,
    Joern> i686-pc-linux-gnulibc1 was affected.

I agree that a redesign is probably in order.  But, we have to get
there in an orderly fashion.  If it's really a large effort/rewrite,
then it might make sense to do what RTH has done with IA32.  You could
work on it for a while.  Then we could put it in the tree,
conditionally compiling either your loop optimization rewrite or the
original.  Then, folks could test it until it was stable; then we
could make the new stuff the default.  Might or might not be a good
plan, depending on the circumstances.

    >> I think I'll back the recent loop.c patches out, back to 1.162,
    >> except

    Joern> This is just avoiding triggering latent bugs by putting
    Joern> more bugs in the code.

Perhaps.  It will also save a lot of cycles around the world as people
will stop trying to bootstrap a broken compiler.  

I would much prefer to have your changes in the compiler.  I'm sure
they will improve the compiler.  I just need to know a) what's wrong
now and b) how your fix fixes that.  Please be patient, and take the
time to explain these things to me.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com