Patchwork Fix compute_reloc_for_constant

login
register
mail settings
Submitter Jan Hubicka
Date Jan. 19, 2014, 2:12 a.m.
Message ID <20140119021239.GA22057@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/312361/
State New
Headers show

Comments

Jan Hubicka - Jan. 19, 2014, 2:12 a.m.
Hi,
while comparing LTO and non-LTO builds I noticed that with LTO we produce a lot
more vtables in datal.rel.ro rather than data.rel.ro.local
This is because of partitioning promoting more symbols global. For RTL we make
section decisions based on SYMBOL_REF_LOCAL_FLAG that is set based on
decl_binds_local_p.  For variables we use TREE_PUBLIC check that is overly
conservative.

Bootstrapped/regtested x86_64-linux, OK?
	* varasm.c (compute_reloc_for_constant): Use targetm.binds_local_p
	instead of TREE_PUBLIC to determine if reference will be local
	within given DSO or not.
aldot - Jan. 19, 2014, 9:05 a.m.
On 19 January 2014 03:12:56 Jan Hubicka <hubicka@ucw.cz> wrote:

> Hi,
> while comparing LTO and non-LTO builds I noticed that with LTO we produce a lot
> more vtables in datal.rel.ro rather than data.rel.ro.local
> This is because of partitioning promoting more symbols global. For RTL we make
> section decisions based on SYMBOL_REF_LOCAL_FLAG that is set based on
> decl_binds_local_p.  For variables we use TREE_PUBLIC check that is overly
> conservative.

Honza,

Would you (or anybody else for that matter) mind looking into PR32219 while 
there?
See http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html and other 
discussion of that bug on the ML last year.

TIA,
Bernhard
>
> Bootstrapped/regtested x86_64-linux, OK?
> 	* varasm.c (compute_reloc_for_constant): Use targetm.binds_local_p
> 	instead of TREE_PUBLIC to determine if reference will be local
> 	within given DSO or not.
> Index: varasm.c
> ===================================================================
> --- varasm.c	(revision 206684)
> +++ varasm.c	(working copy)
> @@ -4060,7 +4060,7 @@
>  	  break;
>  	}
>
> -      if (TREE_PUBLIC (tem))
> +      if (!targetm.binds_local_p (tem))
>  	reloc |= 2;
>        else
>  	reloc |= 1;



Sent with AquaMail for Android
http://www.aqua-mail.com
Richard Guenther - Jan. 19, 2014, 1:55 p.m.
On Sun, Jan 19, 2014 at 3:12 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> while comparing LTO and non-LTO builds I noticed that with LTO we produce a lot
> more vtables in datal.rel.ro rather than data.rel.ro.local
> This is because of partitioning promoting more symbols global. For RTL we make
> section decisions based on SYMBOL_REF_LOCAL_FLAG that is set based on
> decl_binds_local_p.  For variables we use TREE_PUBLIC check that is overly
> conservative.
>
> Bootstrapped/regtested x86_64-linux, OK?

Ok.

Thanks,
Richard.

>         * varasm.c (compute_reloc_for_constant): Use targetm.binds_local_p
>         instead of TREE_PUBLIC to determine if reference will be local
>         within given DSO or not.
> Index: varasm.c
> ===================================================================
> --- varasm.c    (revision 206684)
> +++ varasm.c    (working copy)
> @@ -4060,7 +4060,7 @@
>           break;
>         }
>
> -      if (TREE_PUBLIC (tem))
> +      if (!targetm.binds_local_p (tem))
>         reloc |= 2;
>        else
>         reloc |= 1;
Jan Hubicka - Jan. 23, 2014, 2:43 a.m.
> On 19 January 2014 03:12:56 Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> >Hi,
> >while comparing LTO and non-LTO builds I noticed that with LTO we produce a lot
> >more vtables in datal.rel.ro rather than data.rel.ro.local
> >This is because of partitioning promoting more symbols global. For RTL we make
> >section decisions based on SYMBOL_REF_LOCAL_FLAG that is set based on
> >decl_binds_local_p.  For variables we use TREE_PUBLIC check that is overly
> >conservative.
> 
> Honza,
> 
> Would you (or anybody else for that matter) mind looking into
> PR32219 while there?
> See http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html and
> other discussion of that bug on the ML last year.

This patch is IMO wrong for the reasons Jakub describe (the hidden symbol
must bind local).  Optimizer miss the fact that the symbol is weak and thus
it may bind to NULL.  
I was looking into the folder and the logic there is indeed sloppy (have somewhere
WIP patch to rewrite it), see DECL_WEAK tests in fold-const.c

Honza
> 
> TIA,
> Bernhard
> >
> >Bootstrapped/regtested x86_64-linux, OK?
> >	* varasm.c (compute_reloc_for_constant): Use targetm.binds_local_p
> >	instead of TREE_PUBLIC to determine if reference will be local
> >	within given DSO or not.
> >Index: varasm.c
> >===================================================================
> >--- varasm.c	(revision 206684)
> >+++ varasm.c	(working copy)
> >@@ -4060,7 +4060,7 @@
> > 	  break;
> > 	}
> >
> >-      if (TREE_PUBLIC (tem))
> >+      if (!targetm.binds_local_p (tem))
> > 	reloc |= 2;
> >       else
> > 	reloc |= 1;
> 
> 
> 
> Sent with AquaMail for Android
> http://www.aqua-mail.com
>

Patch

Index: varasm.c
===================================================================
--- varasm.c	(revision 206684)
+++ varasm.c	(working copy)
@@ -4060,7 +4060,7 @@ 
 	  break;
 	}
 
-      if (TREE_PUBLIC (tem))
+      if (!targetm.binds_local_p (tem))
 	reloc |= 2;
       else
 	reloc |= 1;