Patchwork stabilize switch labels for -fcompare-debug in vrp

login
register
mail settings
Submitter Alexandre Oliva
Date Oct. 11, 2010, 1:05 p.m.
Message ID <or4ocsvq0e.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/67419/
State New
Headers show

Comments

Alexandre Oliva - Oct. 11, 2010, 1:05 p.m.
i386.c fails -fcompare-debug at -O3 because label half: in
ix86_expand_vector_init_general gets the DECL_UIDs of labels sorted
differently by the tree-vrp sort function.

As a result, different SSA_NAMEs are used for the assert variables in
different cases of the switch.  At the end of VRP, they're released, and
put in the freelist in different order.  At a later pass, the SSA_NAME
nodes are reused in a different order, and we go down the hill from
there, because different version numbers cause other optimization
differences.

Using LABEL_DECL_UIDs instead of DECL_UIDs gets us a stable sort.  I
haven't figured out why the named label got a different UID.  I only
observed this in the 4.5 branch, but the same patch applies cleanly in
the trunk, and I regstrapped the patch on both trees, on x86 and on
x86_64.

Ok to install?
Richard Guenther - Oct. 11, 2010, 2:09 p.m.
On Mon, Oct 11, 2010 at 3:05 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> i386.c fails -fcompare-debug at -O3 because label half: in
> ix86_expand_vector_init_general gets the DECL_UIDs of labels sorted
> differently by the tree-vrp sort function.
>
> As a result, different SSA_NAMEs are used for the assert variables in
> different cases of the switch.  At the end of VRP, they're released, and
> put in the freelist in different order.  At a later pass, the SSA_NAME
> nodes are reused in a different order, and we go down the hill from
> there, because different version numbers cause other optimization
> differences.
>
> Using LABEL_DECL_UIDs instead of DECL_UIDs gets us a stable sort.  I
> haven't figured out why the named label got a different UID.  I only
> observed this in the 4.5 branch, but the same patch applies cleanly in
> the trunk, and I regstrapped the patch on both trees, on x86 and on
> x86_64.
>
> Ok to install?

Well.  Quoting tree.h:

/* A numeric unique identifier for a LABEL_DECL.  The UID allocation is
   dense, unique within any one function, and may be used to index arrays.
   If the value is -1, then no UID has been assigned.  */
#define LABEL_DECL_UID(NODE) \
  (LABEL_DECL_CHECK (NODE)->label_decl.label_decl_uid)

and if the labels get a different DECL_UID I see no reason why they
shouldn't get a different LABEL_DECL_UID.

So no, I don't think this is an improvement.  Instead we need to understand
why the LABEL_DECL gets a different uid.

Richard.
Jakub Jelinek - Oct. 11, 2010, 2:12 p.m.
On Mon, Oct 11, 2010 at 04:09:05PM +0200, Richard Guenther wrote:
> So no, I don't think this is an improvement.  Instead we need to understand
> why the LABEL_DECL gets a different uid.

Well, a different uid shouldn't be a bug, because -g/-g0 can have
DECL_UID differences, but only by having different gaps between
uids of corresponding decls.  qsort by DECL_UID should still
be the same.

	Jakub
Richard Guenther - Oct. 11, 2010, 2:21 p.m.
On Mon, Oct 11, 2010 at 4:12 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 11, 2010 at 04:09:05PM +0200, Richard Guenther wrote:
>> So no, I don't think this is an improvement.  Instead we need to understand
>> why the LABEL_DECL gets a different uid.
>
> Well, a different uid shouldn't be a bug, because -g/-g0 can have
> DECL_UID differences, but only by having different gaps between
> uids of corresponding decls.  qsort by DECL_UID should still
> be the same.

Of course.  I assumed the get mixed-up from LXOs description.

Richard.

>        Jakub
>
Alexandre Oliva - Oct. 13, 2010, 4:50 a.m.
On Oct 11, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Mon, Oct 11, 2010 at 3:05 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> Using LABEL_DECL_UIDs instead of DECL_UIDs gets us a stable sort.

> and if the labels get a different DECL_UID I see no reason why they
> shouldn't get a different LABEL_DECL_UID.

> So no, I don't think this is an improvement.

Heh.  It was actually your suggestion, although I hadn't seen it yet :-)

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44832#c35

This is just bug 44832, whose fix hadn't been backported to 4.5.  I'll
test it and install it, unless I hear objections in the next 24 hours or
so.
Richard Guenther - Oct. 13, 2010, 9:03 a.m.
On Wed, Oct 13, 2010 at 6:50 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Oct 11, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> On Mon, Oct 11, 2010 at 3:05 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> Using LABEL_DECL_UIDs instead of DECL_UIDs gets us a stable sort.
>
>> and if the labels get a different DECL_UID I see no reason why they
>> shouldn't get a different LABEL_DECL_UID.
>
>> So no, I don't think this is an improvement.
>
> Heh.  It was actually your suggestion, although I hadn't seen it yet :-)
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44832#c35
>
> This is just bug 44832, whose fix hadn't been backported to 4.5.  I'll
> test it and install it, unless I hear objections in the next 24 hours or
> so.

Ah indeed, that makes sense.

Richard.

> --
> 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
>

Patch

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

	* tree-vrp.c (compare_case_labels): Use LABEL_DECL_UID.

Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c.orig	2010-10-08 07:36:36.668853265 -0300
+++ gcc/tree-vrp.c	2010-10-08 07:42:52.872769933 -0300
@@ -4658,8 +4658,8 @@  compare_case_labels (const void *p1, con
 {
   const_tree const case1 = *(const_tree const*)p1;
   const_tree const case2 = *(const_tree const*)p2;
-  unsigned int uid1 = DECL_UID (CASE_LABEL (case1));
-  unsigned int uid2 = DECL_UID (CASE_LABEL (case2));
+  unsigned int uid1 = LABEL_DECL_UID (CASE_LABEL (case1));
+  unsigned int uid2 = LABEL_DECL_UID (CASE_LABEL (case2));
 
   if (uid1 < uid2)
     return -1;