Patchwork Partial inlining

login
register
mail settings
Submitter Jan Hubicka
Date June 27, 2010, 8:02 p.m.
Message ID <20100627200203.GA21666@atrey.karlin.mff.cuni.cz>
Download mbox | patch
Permalink /patch/57101/
State New
Headers show

Comments

Jan Hubicka - June 27, 2010, 8:02 p.m.
> > I still see many libstdc++ run-time failures on Linux/ia32 even with this fix:
> > 
> > http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02683.html
> > 
> > GOLD isn't the default linker and gcc is configured with
> > 
> > --enable-clocale=gnu --with-system-zlib --enable-shared
> > --with-demangler-in-ld -with-plugin-ld=ld.gold --enable-gold
> > --with-fpmath=sse
> 
> Curiosly I get a lot of excess errors that seem unrealated to the problem.  A
> lot of the testcases you mention just pass for me.
> 
> I've reproduced 4 execution failures that go away with -fno-partial-inlining.
> I think it is latent bug in clonning WRT builtin implementations in glibc
> headers.  I am just testing patch for that.

Actually those failures are not dependent on partial inlining either.  The difference
was that I sent the box out of memory in parallel session that killed the testsuite
so it did not finish.

My best bet to fix this is the following patch I am bootstrapping/regtesting now and
will commit once it complette.  I think it depends on glibc version, but some of them
use inlines for string functions that should provoke this bug in function splitting.

Paolo: Hope that the issues are solved now, but if you will still see libstdc++
breakage due to the partial inlining, you can disable it with
-fno-partial-inlining (or by patching out the flag_partial_inlining set in
opts.c).

Honza

	PR middle-end/44671
	PR middle-end/44686
	* tree.c (build_function_decl_skip_args): Clear DECL_BUILT_IN on signature
	change.
	* ipa-split.c (split_function): Always clear DECL_BUILT_IN.
	* ipa-prop.c (ipa_modify_formal_parameters): Likewise.

	* testsuite/gcc.c-torture/pr44686.c: New file.

void *
memcpy (void *a, const void *b, __SIZE_TYPE__ len)
{
  if (a == b)
    __builtin_abort ();
}
Paolo Carlini - June 27, 2010, 8:14 p.m.
On 06/27/2010 10:02 PM, Jan Hubicka wrote:
> Paolo: Hope that the issues are solved now, but if you will still see
> libstdc++
> breakage due to the partial inlining, you can disable it with
> -fno-partial-inlining (or by patching out the flag_partial_inlining set in
> opts.c).
>   
Thanks. Normally I regtest on x86_64-linux -m64 and I can confirm that
for that specific target things are back to normality. If I notice
something useful for -m32, I'll let you know immediately.

Paolo.
Steven Bosscher - June 27, 2010, 8:17 p.m.
On Sun, Jun 27, 2010 at 10:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Index: ipa-split.c
> ===================================================================
> --- ipa-split.c (revision 161472)
> +++ ipa-split.c (working copy)
> @@ -843,6 +843,14 @@ split_function (struct split_point *spli
>                                     args_to_skip,
>                                     split_point->split_bbs,
>                                     split_point->entry_bb, "_part");
> +  /* For usual clonning it is enough to clear builtin only when signature

typo: s/clonning/cloning/

Ciao!
Steven
Richard Guenther - June 28, 2010, 8:30 a.m.
On Sun, 27 Jun 2010, Jan Hubicka wrote:

> > > I still see many libstdc++ run-time failures on Linux/ia32 even with this fix:
> > > 
> > > http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02683.html
> > > 
> > > GOLD isn't the default linker and gcc is configured with
> > > 
> > > --enable-clocale=gnu --with-system-zlib --enable-shared
> > > --with-demangler-in-ld -with-plugin-ld=ld.gold --enable-gold
> > > --with-fpmath=sse
> > 
> > Curiosly I get a lot of excess errors that seem unrealated to the problem.  A
> > lot of the testcases you mention just pass for me.
> > 
> > I've reproduced 4 execution failures that go away with -fno-partial-inlining.
> > I think it is latent bug in clonning WRT builtin implementations in glibc
> > headers.  I am just testing patch for that.
> 
> Actually those failures are not dependent on partial inlining either.  The difference
> was that I sent the box out of memory in parallel session that killed the testsuite
> so it did not finish.
> 
> My best bet to fix this is the following patch I am bootstrapping/regtesting now and
> will commit once it complette.  I think it depends on glibc version, but some of them
> use inlines for string functions that should provoke this bug in function splitting.
> 
> Paolo: Hope that the issues are solved now, but if you will still see libstdc++
> breakage due to the partial inlining, you can disable it with
> -fno-partial-inlining (or by patching out the flag_partial_inlining set in
> opts.c).
> 
> Honza

Can you install relevant parts also on the 4.5 branch?

Thanks,
Richard.

> 	PR middle-end/44671
> 	PR middle-end/44686
> 	* tree.c (build_function_decl_skip_args): Clear DECL_BUILT_IN on signature
> 	change.
> 	* ipa-split.c (split_function): Always clear DECL_BUILT_IN.
> 	* ipa-prop.c (ipa_modify_formal_parameters): Likewise.
> 
> 	* testsuite/gcc.c-torture/pr44686.c: New file.
> 
> void *
> memcpy (void *a, const void *b, __SIZE_TYPE__ len)
> {
>   if (a == b)
>     __builtin_abort ();
> }
> 
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 161471)
> +++ tree.c	(working copy)
> @@ -7303,6 +7303,13 @@ build_function_decl_skip_args (tree orig
>       we expect first argument to be THIS pointer.   */
>    if (bitmap_bit_p (args_to_skip, 0))
>      DECL_VINDEX (new_decl) = NULL_TREE;
> +
> +  /* When signature changes, we need to clear builtin info.  */
> +  if (DECL_BUILT_IN (new_decl) && !bitmap_empty_p (args_to_skip))
> +    {
> +      DECL_BUILT_IN_CLASS (new_decl) = NOT_BUILT_IN;
> +      DECL_FUNCTION_CODE (new_decl) = (enum built_in_function) 0;
> +    }
>    return new_decl;
>  }
>  
> Index: ipa-split.c
> ===================================================================
> --- ipa-split.c	(revision 161472)
> +++ ipa-split.c	(working copy)
> @@ -843,6 +843,14 @@ split_function (struct split_point *spli
>  				     args_to_skip,
>  				     split_point->split_bbs,
>  				     split_point->entry_bb, "_part");
> +  /* For usual clonning it is enough to clear builtin only when signature
> +     changes.  For partial inlining we however can not expect the part
> +     of builtin implementation to have same semantic as the whole.  */
> +  if (DECL_BUILT_IN (node->decl))
> +    {
> +      DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
> +      DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
> +    }
>    cgraph_node_remove_callees (cgraph_node (current_function_decl));
>    if (!split_part_return_p)
>      TREE_THIS_VOLATILE (node->decl) = 1;
> Index: ipa-prop.c
> ===================================================================
> --- ipa-prop.c	(revision 161471)
> +++ ipa-prop.c	(working copy)
> @@ -2087,6 +2087,13 @@ ipa_modify_formal_parameters (tree fndec
>        DECL_VINDEX (fndecl) = NULL_TREE;
>      }
>  
> +  /* When signature changes, we need to clear builtin info.  */
> +  if (DECL_BUILT_IN (fndecl) && !bitmap_empty_p (args_to_skip))
> +    {
> +      DECL_BUILT_IN_CLASS (fndecl) = NOT_BUILT_IN;
> +      DECL_FUNCTION_CODE (fndecl) = (enum built_in_function) 0;
> +    }
> +
>    /* This is a new type, not a copy of an old type.  Need to reassociate
>       variants.  We can handle everything except the main variant lazily.  */
>    t = TYPE_MAIN_VARIANT (orig_type);
> 
>
Paolo Carlini - June 28, 2010, 9:02 a.m.
On 06/27/2010 10:14 PM, Paolo Carlini wrote:
> On 06/27/2010 10:02 PM, Jan Hubicka wrote:
>> Paolo: Hope that the issues are solved now, but if you will still see
>> libstdc++
>> breakage due to the partial inlining, you can disable it with
>> -fno-partial-inlining (or by patching out the flag_partial_inlining set in
>> opts.c).
>>   
> Thanks. Normally I regtest on x86_64-linux -m64 and I can confirm that
> for that specific target things are back to normality. If I notice
> something useful for -m32, I'll let you know immediately.

As reported already by others (see HJ's testresults) unfortunately I can
also confirm that -m32 is still broken: abi_check fails immediately but
the setup of the machine is fine, it's simply miscompiled. As for the
glibc version, nothing special, a 2.10.1.

Paolo.
Jan Hubicka - June 28, 2010, 12:39 p.m.
> On 06/27/2010 10:14 PM, Paolo Carlini wrote:
> > On 06/27/2010 10:02 PM, Jan Hubicka wrote:
> >> Paolo: Hope that the issues are solved now, but if you will still see
> >> libstdc++
> >> breakage due to the partial inlining, you can disable it with
> >> -fno-partial-inlining (or by patching out the flag_partial_inlining set in
> >> opts.c).
> >>   
> > Thanks. Normally I regtest on x86_64-linux -m64 and I can confirm that
> > for that specific target things are back to normality. If I notice
> > something useful for -m32, I'll let you know immediately.
> 
> As reported already by others (see HJ's testresults) unfortunately I can
> also confirm that -m32 is still broken: abi_check fails immediately but
> the setup of the machine is fine, it's simply miscompiled. As for the
> glibc version, nothing special, a 2.10.1.

I just posted fix for NRV problem that at least has the property that the testcase
reproduces on 32 bit compilation only.
Does the problem reproduce for you with --test-board=unit,-m32 on 64bit build?

Honza
> 
> Paolo.
Paolo Carlini - June 28, 2010, 1:12 p.m.
On 06/28/2010 02:39 PM, Jan Hubicka wrote:
> I just posted fix for NRV problem that at least has the property that
> the testcase
> reproduces on 32 bit compilation only.
> Does the problem reproduce for you with --test-board=unit,-m32 on 64bit build?
>   
Yes, I don't have ready at hand a real 32-bit machine, like an i686, I'm
seeing exactly what JH is seeing on testresults, thus problems for
x86_64-linux -m32:

    http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02884.html

Now I'm in the middle of a couple other things, but I can test your NRV
patch soon.

Thanks!
Paolo.
Paolo Carlini - June 28, 2010, 1:25 p.m.
On 06/28/2010 03:12 PM, Paolo Carlini wrote:
> Now I'm in the middle of a couple other things, but I can test your NRV
> patch soon.
>   
I quickly rebuilt with the NRV patch applied and unfortunately the
abi-check miscompilation for -m32 is still there. The rest of the
testsuite is running...

Paolo.

Patch

Index: tree.c
===================================================================
--- tree.c	(revision 161471)
+++ tree.c	(working copy)
@@ -7303,6 +7303,13 @@  build_function_decl_skip_args (tree orig
      we expect first argument to be THIS pointer.   */
   if (bitmap_bit_p (args_to_skip, 0))
     DECL_VINDEX (new_decl) = NULL_TREE;
+
+  /* When signature changes, we need to clear builtin info.  */
+  if (DECL_BUILT_IN (new_decl) && !bitmap_empty_p (args_to_skip))
+    {
+      DECL_BUILT_IN_CLASS (new_decl) = NOT_BUILT_IN;
+      DECL_FUNCTION_CODE (new_decl) = (enum built_in_function) 0;
+    }
   return new_decl;
 }
 
Index: ipa-split.c
===================================================================
--- ipa-split.c	(revision 161472)
+++ ipa-split.c	(working copy)
@@ -843,6 +843,14 @@  split_function (struct split_point *spli
 				     args_to_skip,
 				     split_point->split_bbs,
 				     split_point->entry_bb, "_part");
+  /* For usual clonning it is enough to clear builtin only when signature
+     changes.  For partial inlining we however can not expect the part
+     of builtin implementation to have same semantic as the whole.  */
+  if (DECL_BUILT_IN (node->decl))
+    {
+      DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
+      DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
+    }
   cgraph_node_remove_callees (cgraph_node (current_function_decl));
   if (!split_part_return_p)
     TREE_THIS_VOLATILE (node->decl) = 1;
Index: ipa-prop.c
===================================================================
--- ipa-prop.c	(revision 161471)
+++ ipa-prop.c	(working copy)
@@ -2087,6 +2087,13 @@  ipa_modify_formal_parameters (tree fndec
       DECL_VINDEX (fndecl) = NULL_TREE;
     }
 
+  /* When signature changes, we need to clear builtin info.  */
+  if (DECL_BUILT_IN (fndecl) && !bitmap_empty_p (args_to_skip))
+    {
+      DECL_BUILT_IN_CLASS (fndecl) = NOT_BUILT_IN;
+      DECL_FUNCTION_CODE (fndecl) = (enum built_in_function) 0;
+    }
+
   /* This is a new type, not a copy of an old type.  Need to reassociate
      variants.  We can handle everything except the main variant lazily.  */
   t = TYPE_MAIN_VARIANT (orig_type);