fix -fcompare-debug regression in free_lang_data

fix -fcompare-debug regression in free_lang_data

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

The free_lang_data pass, called when not emitting debug info, introduces
new alias sets for select data types that hadn't had been assigned an
alias set yet.  This gets alias set numbers out of sync.  I don't know
of any codegen differences that follow from this, but -fcompare-debug
fails because of the different alias set numbers in MEMs.

This patch arranges for free_lang_data() to not depend on whether or not
we're emitting debug information, removing an extraneous reference to
flag_gtoggle while at that.  flag_gtoggle is only to be used when
processing command-line options, and it takes effect by changing the
debug “verbosity” level.  I can't think of any legitimate reason to
check its value elsewhere.

Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-no-free-lang-data.patch --]
[-- Type: text/x-diff, Size: 860 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* tree.c (gate_free_lang_data): Don't depend on debug info level.

Index: gcc/tree.c
===================================================================
--- gcc/tree.c.orig	2009-10-16 04:09:35.000000000 -0300
+++ gcc/tree.c	2009-10-16 04:12:06.000000000 -0300
@@ -4977,10 +4977,11 @@ free_lang_data (void)
 static bool
 gate_free_lang_data (void)
 {
-  /* FIXME.  Remove after save_debug_info is working.  */
-  return (flag_generate_lto
-	  || (!in_lto_p
-	      && !flag_gtoggle && debug_info_level <= DINFO_LEVEL_TERSE));
+  /* FIXME.  Remove after save_debug_info is working.  Don't depend on
+     debug level, it breaks -fcompare-debug because it changes the
+     numbering of alias sets calling get_alias_set within
+     free_lang_data_in_type().  */
+  return flag_generate_lto;
 }
 
 

[-- Attachment #3: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: fix -fcompare-debug regression in free_lang_data

[Richard Guenther]

On Fri, Oct 16, 2009 at 9:42 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> The free_lang_data pass, called when not emitting debug info, introduces
> new alias sets for select data types that hadn't had been assigned an
> alias set yet.  This gets alias set numbers out of sync.  I don't know
> of any codegen differences that follow from this, but -fcompare-debug
> fails because of the different alias set numbers in MEMs.
>
> This patch arranges for free_lang_data() to not depend on whether or not
> we're emitting debug information, removing an extraneous reference to
> flag_gtoggle while at that.  flag_gtoggle is only to be used when
> processing command-line options, and it takes effect by changing the
> debug “verbosity” level.  I can't think of any legitimate reason to
> check its value elsewhere.
>
> Ok to install?

No.  The point was to have free-lang-data a non-LTO specific
thing.  Can you try to instead simply use flag_no_strict_aliasing
|| lang_hooks.get_alias_set () instead of get_alias_set()?  The
point here is to catch C-style 'char' and units built with
no-strict-aliasing mixed with ones that do not.

Thanks,
Richard.

Re: fix -fcompare-debug regression in free_lang_data

[Alexandre Oliva]

On Oct 16, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Fri, Oct 16, 2009 at 9:42 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> The free_lang_data pass, called when not emitting debug info, introduces
>> new alias sets for select data types that hadn't had been assigned an
>> alias set yet.  This gets alias set numbers out of sync.  I don't know
>> of any codegen differences that follow from this, but -fcompare-debug
>> fails because of the different alias set numbers in MEMs.
>> 
>> This patch arranges for free_lang_data() to not depend on whether or not
>> we're emitting debug information, removing an extraneous reference to
>> flag_gtoggle while at that.  flag_gtoggle is only to be used when
>> processing command-line options, and it takes effect by changing the
>> debug “verbosity” level.  I can't think of any legitimate reason to
>> check its value elsewhere.
>> 
>> Ok to install?

> Can you try to instead simply use flag_no_strict_aliasing
> || lang_hooks.get_alias_set () instead of get_alias_set()?

No, that's not enough.  We already call lang_hooks.get_alias_set().

But language-specific get_alias_set() may (and do) call get_alias_set()
for types such as unsigned versions of built-in signed types, and
unqualified versions of pointer types.

IIRC, it's precisely the built-in unsigned types that cause this,
because we call get_alias_set() for the corresponding signed types, for
which we hadn't created an alias set yet.

One simple way around this problem would be to create alias sets for all
built-in types upfront, but there are probably other ways to fix this.

Regardless, testing for flag_gtoggle there doesn't make any sense.  This
is not a flag to base decisions on.

Anyhow, given the above, what do you suggest?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: fix -fcompare-debug regression in free_lang_data

[Richard Guenther]

On Sat, Oct 17, 2009 at 9:09 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Oct 16, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> On Fri, Oct 16, 2009 at 9:42 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> The free_lang_data pass, called when not emitting debug info, introduces
>>> new alias sets for select data types that hadn't had been assigned an
>>> alias set yet.  This gets alias set numbers out of sync.  I don't know
>>> of any codegen differences that follow from this, but -fcompare-debug
>>> fails because of the different alias set numbers in MEMs.
>>>
>>> This patch arranges for free_lang_data() to not depend on whether or not
>>> we're emitting debug information, removing an extraneous reference to
>>> flag_gtoggle while at that.  flag_gtoggle is only to be used when
>>> processing command-line options, and it takes effect by changing the
>>> debug “verbosity” level.  I can't think of any legitimate reason to
>>> check its value elsewhere.
>>>
>>> Ok to install?
>
>> Can you try to instead simply use flag_no_strict_aliasing
>> || lang_hooks.get_alias_set () instead of get_alias_set()?
>
> No, that's not enough.  We already call lang_hooks.get_alias_set().
>
> But language-specific get_alias_set() may (and do) call get_alias_set()
> for types such as unsigned versions of built-in signed types, and
> unqualified versions of pointer types.
>
> IIRC, it's precisely the built-in unsigned types that cause this,
> because we call get_alias_set() for the corresponding signed types, for
> which we hadn't created an alias set yet.
>
> One simple way around this problem would be to create alias sets for all
> built-in types upfront, but there are probably other ways to fix this.
>
> Regardless, testing for flag_gtoggle there doesn't make any sense.  This
> is not a flag to base decisions on.
>
> Anyhow, given the above, what do you suggest?

Well - unconditionally (also with -g) run free-lang-data but do not free things
(or redirect langhooks), only do the get_alias_set thing.  I can try to properly
cook up a nice patch, but you might be able to tell if that would fix
your problem.

Another fix would be to introduce another langhook that just identifies
alias-set-zero types to avoid calling into get_alias_set and exactly do
what is required here.  Or maybe introduce a new type flag, similar
to TYPE_REF_CAN_ALIAS_ALL but for the pointed-to type that marks
the char types.  Or require the languages to statically assign alias-set zero
for these during lang-init.

Does anyone else have an opinion here?  The issue is that the middle-end
has to preserve alias-set zero if the frontend alias_set langhook would
assign that to a type (or reference, but that's a different problem).

Thanks,
Richard.

Re: fix -fcompare-debug regression in free_lang_data

[Alexandre Oliva]

On Oct 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> Well - unconditionally (also with -g) run free-lang-data but do not
> free things (or redirect langhooks), only do the get_alias_set thing.
> I can try to properly cook up a nice patch, but you might be able to
> tell if that would fix your problem.

I see you implemented this.  Thanks.

I see still a problem in Ada.  Maybe it's not visible with the default
bootstrap-debug BUILD_CONFIG, because the incorrect use of flag_gtoggle
hides it, but it's noticeable if you compile with say ada/einfo.adb
(among tens of others) with '-O2 -fcompare-debug=-g' (without any other
-g).

The problem is that the Ada boolean_type is overridden by the different
type set up in free_lang_data(), and then various simplifications that
succeed when the boolean_type remains the same fail because of a type
cast.

For example, in ada/einfo.adb, various compares such as this in
einfo__Dependent_Instances:

  D.#_3 = !D.#_2;
  if (D.#_3 != 0)       

are simplified in forwprop1 to 

  if (D.#_2 == 0)       

if boolean_type_node is unchanged, but if
forward_propagate_into_gimple_cond() calls combine_cond_expr_cond() with
a modified boolean_type_node (e.g. the one set up in free_lang_data()),
the fold_binary_loc() call will return a NOP_EXPR node to convert an
Ada-boolean result to the free_lang_data-set boolean_type_node, and
canonicalize_cond_expr_cond() turns that into a negate and compare that
does not satisfy is_gimple_condexpr().

Now, it would be possible to tweak canonicalize_cond_expr_cond() to
handle a NOP_EXPR of a TRUTH_NOT_EXPR and return an EQ_EXPR, rather than
a NE_EXPR with a TRUTH_NOT_EXPR operand, or perhaps even to get
fold_binary_loc() to avoid the explicit type-cast, but the result would
still have a different type from that of a compilation in which
boolean_type_node is not overridden, so a potential for codegen
differences would remain.

Thoughts?


Regardless, the flag_gtoggle in free_lang_data() must go, it doesn't
make any sense whatsoever, it's doing nothing but papering over this
problem.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: fix -fcompare-debug regression in free_lang_data

[Richard Guenther]

On Sun, Nov 8, 2009 at 5:06 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Oct 17, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Well - unconditionally (also with -g) run free-lang-data but do not
>> free things (or redirect langhooks), only do the get_alias_set thing.
>> I can try to properly cook up a nice patch, but you might be able to
>> tell if that would fix your problem.
>
> I see you implemented this.  Thanks.
>
> I see still a problem in Ada.  Maybe it's not visible with the default
> bootstrap-debug BUILD_CONFIG, because the incorrect use of flag_gtoggle
> hides it, but it's noticeable if you compile with say ada/einfo.adb
> (among tens of others) with '-O2 -fcompare-debug=-g' (without any other
> -g).
>
> The problem is that the Ada boolean_type is overridden by the different
> type set up in free_lang_data(), and then various simplifications that
> succeed when the boolean_type remains the same fail because of a type
> cast.
>
> For example, in ada/einfo.adb, various compares such as this in
> einfo__Dependent_Instances:
>
>  D.#_3 = !D.#_2;
>  if (D.#_3 != 0)
>
> are simplified in forwprop1 to
>
>  if (D.#_2 == 0)
>
> if boolean_type_node is unchanged, but if
> forward_propagate_into_gimple_cond() calls combine_cond_expr_cond() with
> a modified boolean_type_node (e.g. the one set up in free_lang_data()),
> the fold_binary_loc() call will return a NOP_EXPR node to convert an
> Ada-boolean result to the free_lang_data-set boolean_type_node, and
> canonicalize_cond_expr_cond() turns that into a negate and compare that
> does not satisfy is_gimple_condexpr().
>
> Now, it would be possible to tweak canonicalize_cond_expr_cond() to
> handle a NOP_EXPR of a TRUTH_NOT_EXPR and return an EQ_EXPR, rather than
> a NE_EXPR with a TRUTH_NOT_EXPR operand, or perhaps even to get
> fold_binary_loc() to avoid the explicit type-cast, but the result would
> still have a different type from that of a compilation in which
> boolean_type_node is not overridden, so a potential for codegen
> differences would remain.

Can you open a bugreport for this?  I'd like to have a look (but I'll be
unavailable the next week, so I'll likely forget).

> Thoughts?
>
>
> Regardless, the flag_gtoggle in free_lang_data() must go, it doesn't
> make any sense whatsoever, it's doing nothing but papering over this
> problem.

Hm, no idea why it was there in the first place.  A patch to remove it
is pre-approved (if that passes bootstrap & regtest of course).

Thanks,
Richard.

Re: fix -fcompare-debug regression in free_lang_data

[Eric Botcazou]

> Hm, no idea why it was there in the first place.  A patch to remove it
> is pre-approved (if that passes bootstrap & regtest of course).

Because of the FIXME:

  /* FIXME.  Remove after save_debug_info is working.  */
  if (!(flag_generate_lto
	|| (!in_lto_p
	    && !flag_gtoggle && debug_info_level <= DINFO_LEVEL_TERSE)))
    return 0;

If you remove the flag_gtoggle test, you will end up generating different code 
with -gtoggle -g0 and -gtoggle -g for the languages whose boolean_type_node 
is changed, thus breaking bootstrap comparison.

Of course, as Alexandre said, that would probably be a good thing because 
different code is already generated for these languages at -g and -g0 due to 
the boolean_type_node hack, but it's hidden by -gtoggle.

-- 
Eric Botcazou

Re: fix -fcompare-debug regression in free_lang_data

[Richard Guenther]

On Sun, Nov 8, 2009 at 10:57 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Hm, no idea why it was there in the first place.  A patch to remove it
>> is pre-approved (if that passes bootstrap & regtest of course).
>
> Because of the FIXME:
>
>  /* FIXME.  Remove after save_debug_info is working.  */
>  if (!(flag_generate_lto
>        || (!in_lto_p
>            && !flag_gtoggle && debug_info_level <= DINFO_LEVEL_TERSE)))
>    return 0;
>
> If you remove the flag_gtoggle test, you will end up generating different code
> with -gtoggle -g0 and -gtoggle -g for the languages whose boolean_type_node
> is changed, thus breaking bootstrap comparison.
>
> Of course, as Alexandre said, that would probably be a good thing because
> different code is already generated for these languages at -g and -g0 due to
> the boolean_type_node hack, but it's hidden by -gtoggle.

Well, sure.  But that's clearly a bug (no, not the boolean_type_node feature,
but the different code generation).

Richard.

> --
> Eric Botcazou
>

Re: fix -fcompare-debug regression in free_lang_data

[Eric Botcazou]

> Well, sure.  But that's clearly a bug (no, not the boolean_type_node
> feature, but the different code generation).

It's as clear a bug as the boolean_type_node thing is a feature. :-)

-- 
Eric Botcazou

Re: fix -fcompare-debug regression in free_lang_data

[Alexandre Oliva]

On Nov  8, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:

> I see still a problem in Ada.  Maybe it's not visible with the default
> bootstrap-debug BUILD_CONFIG, because the incorrect use of flag_gtoggle
> hides it, but it's noticeable if you compile with say ada/einfo.adb
> (among tens of others) with '-O2 -fcompare-debug=-g' (without any other
> -g).

Another -fcompare-debug problem is that free_lang_data_in_block()
removes types from BLOCK_VARSes, and decl uids go out of sync depending
on whether a function is inlined with or without this transformation.
Out-of-sync decl uids has been a great recipe for codegen differences,
so lto needs to fix this somehow.

This problem is visible compiling tree.c itself with -g
-fcompare-debug=-g0: it's when gimple_op gets inlined into
find_decls_types_in_node that decl uids go out of sync, because the
CONST_CAST-introduced union is not there in the -g0 compilation, and it
remains in place in the -g compilation.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: fix -fcompare-debug regression in free_lang_data

[Richard Guenther]

On Mon, Nov 9, 2009 at 1:29 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov  8, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> I see still a problem in Ada.  Maybe it's not visible with the default
>> bootstrap-debug BUILD_CONFIG, because the incorrect use of flag_gtoggle
>> hides it, but it's noticeable if you compile with say ada/einfo.adb
>> (among tens of others) with '-O2 -fcompare-debug=-g' (without any other
>> -g).
>
> Another -fcompare-debug problem is that free_lang_data_in_block()
> removes types from BLOCK_VARSes, and decl uids go out of sync depending
> on whether a function is inlined with or without this transformation.
> Out-of-sync decl uids has been a great recipe for codegen differences,
> so lto needs to fix this somehow.
>
> This problem is visible compiling tree.c itself with -g
> -fcompare-debug=-g0: it's when gimple_op gets inlined into
> find_decls_types_in_node that decl uids go out of sync, because the
> CONST_CAST-introduced union is not there in the -g0 compilation, and it
> remains in place in the -g compilation.

At least non-variadic types could all go to non-localized vars, no?  Honza?

Richard.

Re: fix -fcompare-debug regression in free_lang_data

[Alexandre Oliva]

On Nov  8, 2009, Eric Botcazou <ebotcazou@adacore.com> wrote:

>> A patch to remove it is pre-approved (if that passes bootstrap &
>> regtest of course).

> If you remove the flag_gtoggle test, you will end up generating
> different code with -gtoggle -g0 and -gtoggle -g for the languages
> whose boolean_type_node is changed, thus breaking bootstrap
> comparison.

Yep.  I can get bootstrap compare to pass using a non-default
BUILD_CONFIG, but with bootstrap-debug it fails as expected.  Should I
check it in and expose the existing breakage, or leave it papered over?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: fix -fcompare-debug regression in free_lang_data

[Eric Botcazou]

> Yep.  I can get bootstrap compare to pass using a non-default
> BUILD_CONFIG, but with bootstrap-debug it fails as expected.  Should I
> check it in and expose the existing breakage, or leave it papered over?

The problem is that we don't seem to agree as to what the cause is exactly so 
I'm afraid nobody will step in and fix the bootstrap failure.

-- 
Eric Botcazou

Re: fix -fcompare-debug regression in free_lang_data

[Richard Guenther]

On Tue, Nov 17, 2009 at 9:02 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yep.  I can get bootstrap compare to pass using a non-default
>> BUILD_CONFIG, but with bootstrap-debug it fails as expected.  Should I
>> check it in and expose the existing breakage, or leave it papered over?
>
> The problem is that we don't seem to agree as to what the cause is exactly so
> I'm afraid nobody will step in and fix the bootstrap failure.

Please somebody file a bugreport with a testcase for the
debug-compare failure.  I will look into it.

Richard.

Re: fix -fcompare-debug regression in free_lang_data

[Alexandre Oliva]

On Nov  8, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Sun, Nov 8, 2009 at 5:06 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> The problem is that the Ada boolean_type is overridden by the different
>> type set up in free_lang_data(), and then various simplifications that
>> succeed when the boolean_type remains the same fail because of a type
>> cast.

> Can you open a bugreport for this?

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42088

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer