Re: Put scope blocks on a diet

[Alexandre Oliva <aoliva at redhat dot com>]

On Nov 28, 2007, "Richard Guenther" <richard.guenther@gmail.com> wrote:

> on top of current trunk (which includes your patch).  And (no surprise here)
> the regression is completely gone (remember, this is a non-debug build).

Thanks for checking.  Just to be clear, you're talking about the
memory-reduction regression, not the -g-mustn't-change-code
regression, right?

>> On what grounds do you disregard maek bootstrap-debug as a testcase?
>> Do you know of any other mechanisms we have to detect changes in
>> generated code caused by enabling debug information?

> Well, a testcase is something that can be checked in the testsuite to
> see if it regresses

The test process for any patch involves a bootstrap before the
testsuite is run.  Now that we have infrastructure to test for debug
info, I'd like to see that changed to bootstrap-debug.  It's faster
and it tests this property, with at least as much coverage as a GCC
bootstrap gives us (which is admittedly incomplete).

>> I just don't think it justifies breaking the -g/-g0 principle.

> Well, if we want to make that priciple "absolut", the only sane way to
> implement it is to just always follow the same path during compilation

That's a wasteful way to do it.  The other way to pursue this goal is
to be more efficient when possible, and be watchful for divergencies,
fixing them without hesitation when they show up.  Very much like we'd
do for executable code generation bugs.  In fact, I can see a similar
version of your argument stating that the only sane way to write a
compiler is to not perform any optimizations whatsoever, since every
such transformation can modify the behavior of the program.  But this
nonsensical conclusion shows that there's something wrong with the
premise.

Sure, avoiding bugs and fixing them as soon as possible after they're
discovered is more work, but it's a good trade-off given that we want
an optimizing compiler capable of emitting both executable code and
debug information correctly, and that doesn't modify the executable
code depending on whether debug information is being emitted, for
practical reasons that justified an early design principle that has so
far been unchallenged.

But, just like we use testing to make sure that optimizations don't
break properties we want to maintain in executable code, we can use
testing to make sure that debug information generation doesn't.  Enter
bootstrap-debug.  And the nice thing is that it doesn't only exercise
more paths in the compiler, it is also faster than a full bootstrap
with debug information enabled (the current default).  We could easily
suggest it as the default testing procedure, at least on platforms on
which it works, for I honestly don't know how portable bit-per-bit
comparison of stripped objects is.

Along the same lines, there's the still-ugly collection of 3 patches I
proposed a few days ago, that introduces means to bootstrap-compare
the whole tree, rather than just the gcc sub-directory, for bootstrap
miscompares, and that introduces means to easily perform yet another
build of the target libraries with -g0, to compare with the -g build
and extend the coverage of testing of the -g/-g0 property to other
languages that exercise paths outside those exercised by the portable
subset of C and Ada that our front-, middle- and back-ends rely on.

With this patchset, I could verify that the only difference between -g
and -g0 ATM, in the VTA branch (that has -fvar-tracking-assignments
enabled by default if -g is enabled) on x86_64-linux-gnu is in
gcc/ada/rts/sysdep.o, and it's a nearly-irrelevant one: with -g, some
global static pointers to strings that are unused and thus optimized
away have their initializer strings emitted in non-debug sections, but
they're not emitted at all without -g.

>> Is it going to cause decl uids to vary with or without debug info?
>> This would be very unfortunate and IMHO highly undesirable.

> Yes and no.  Code generated shouldn't depend on varying UIDs.

Do you include debug information under "code generated"?  I honestly
don't know whether decl uids, used as identifiers for purposes of
debug dumps, aren't used in debug information too.  I guess not, but I
haven't verified, and I'm wondering if you did.

> In principle bootstrap comparison should survive even if we did

>     DECL_UID (t) = next_decl_uid;
>     next_decl_uid += random() % 13;

> anything else I consider a bug in how we use DECL_UIDs.

Agreed.

> That the DECL_UIDs printed in the dumps would be different with/without
> debug info is unfortunate, but you can make it less worse by making some
> room for extra UIDs per function (that is, "rounding" to the next 10000
> before emitting the next function).

That helps, for sure, but it makes debugging more painful, because
"next function" is a very fuzzy notion for inlining passes, and for
"time-shared" execution of passes that create new decls for different
functions.  (by time-shared, I mean running some passes for some
functions, then running some passes for other functions, then going
back to the first functions, and even doing so in a different order)

And then, even if you round before each declaration-generating pass,
it would become far more difficult to track divergencies across
inlining.

I don't think keeping DECL_UIDs is much more useful to debugging any
kind of problem other than -g/-g0 divergencies, so this desirable
property shouldn't trample other more important properties, such as
saving memory or making the compiler faster.  But I would like to
achieve similar improvements without destroying this property and
without creating maintainability problems, and I believe this is
possible and practical.

> But surely you now won't make consistent DECL_UIDs in debug dump
> -g/-g0 a correctness issue?

Heh.  No, certainly not.  That's why I chose the words "unfortunate"
and "undesirable", rather than "wrong".

This property is not a fundamental design decision, and we don't have
agreement (or even a proposal thereof) that it's important enough to
be elevated to a strict requirement.  It's merely desirable.  That it
has the nice side effect that we can then use uids for hashing, when
we know the number of entries is stable, is just a bonus, even if a
bonus of debatable value ;-) But we could live without it, for sure.
Even nicer would be some option to *make* decl uids stable, at some
memory penalty.  I.e., some debug-dump option (with similar purpose as
-fdump-unnumbered, that makes rtl dumps easier to compare) that could
be tested along with !cfun->after_inlining at the spot you proposed to
modify, so as to *optionally* keep DECL_UID in sync, thus making TREE
dumps easier to compare.

> Well, you disregard the discussion we had on IRC and you disregard
> mails sent after the inital request to revert (the whole) patch.

I don't disregard them.  By the time we got past it, the damage was
already done.

The request and decision to revert the patch before gathering or
requesting information or giving someone who understood the issue a
chance to explain it was harmful to a collaborative development
community.

I certainly didn't help with my heated reaction, for which I
apologize, but I hope you and Mark can understand how insulted I felt
for not having been consulted before the request and the decision.  It
certainly didn't help that we have an ongoing heated and painful
debate on an issue in which similar terms show up, to the point that
even we have had trouble realizing that this patch was about a
completely different issue.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}