Re: [PATCH] Remove special streaming of builtins

[Richard Biener]

On Wed, 27 Jul 2016, Ilya Enkovich wrote:

> 2016-07-26 22:52 GMT+03:00 Richard Biener :
> > On July 26, 2016 7:26:46 PM GMT+02:00, "H.J. Lu"  
> > wrote:
> >>On Mon, Jul 25, 2016 at 4:35 AM, Richard Biener 
> >>wrote:
> >>>
> >>> So I needed to fix that builtins appearing in BLOCK_VARs and the
> >>solution
> >>> I came up with accidentially disabled streaming via the special path.
> >>> Thus the following patch removes the special-casing completely and
> >>makes
> >>> the BLOCK_VARs handling work the same way as for regular externs (by
> >>> streaming a local copy).  We stream each builtin decl once and then
> >>> refer to it via the decl index (which is cheaper than the special
> >>> casing).
> >>>
> >>> I'm not 100% this solves for example the -fno-math-errno inlining
> >>> across TUs (it certainly doesn't if you use attribute optimize with
> >>> -fno-math-errno), but it eventually should by means of having two
> >>> different BUILT_IN_XXX if they have different behavior.  At least
> >>> if all relevant bits are set on the function _type_ rather than
> >>> the decl which I think we still lto-symtab replace with one
> >>> entity during WPA(?)
> >>>
> >>> Well.
> >>>
> >>> LTO bootstrapped and tested on x86_64-unknown-linux-gnu
> >>(c,c++,fortran),
> >>> bootstrapped on x86_64-unknown-linux-gnu (all), testing in progress.
> >>>
> >>> I might have not catched all fndecl compares.
> >>>
> >>> Will apply to trunk if testing completes.  As said, maybe followup
> >>> cleanups possible, at least to lto-opts.c / lto-wrapper.
> >>>
> >>> Richard.
> >>>
> >>> 2016-07-25  Richard Biener  
> >>>
> >>> * cgraph.c (cgraph_node::verify_node): Compare against
> >>builtin
> >>> by using DECL_BUILT_IN_CLASS and DECL_FUNCTION_CODE.
> >>> * tree-chkp.c (chkp_gimple_call_builtin_p): Likewise.
> >>> * tree-streamer.h (streamer_handle_as_builtin_p): Remove.
> >>> (streamer_get_builtin_tree): Likewise.
> >>> (streamer_write_builtin): Likewise.
> >>> * lto-streamer.h (LTO_builtin_decl): Remove.
> >>> * lto-streamer-in.c (lto_read_tree_1): Remove assert.
> >>> (lto_input_scc): Remove LTO_builtin_decl handling.
> >>> (lto_input_tree_1): Liekwise.
> >>> * lto-streamer-out.c (lto_output_tree_1): Remove special
> >>> handling of builtins.
> >>> (DFS::DFS): Likewise.
> >>> * tree-streamer-in.c (streamer_get_builtin_tree): Remove.
> >>> * tree-streamer-out.c (pack_ts_function_decl_value_fields):
> >>Remove
> >>> assert.
> >>> (streamer_write_builtin): Remove.
> >>>
> >>> lto/
> >>> * lto.c (compare_tree_sccs_1): Remove
> >>streamer_handle_as_builtin_p uses.
> >>> (unify_scc): Likewise.
> >>> (lto_read_decls): Likewise.
> >>>
> >>
> >>This caused:
> >>
> >>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72683
> >
> > Probably another by-decl built-in function compare in the mpx support.  I 
> > have fixed the one that triggered on a not mpx capable machine.  Ilya 
> > possibly knows where the other one(s) are lurking off his head?
> 
> I found a couple more of such comparisons.  Their replacement fixes
> PR72683 and PR72657.

Great!  Thanks for tracking this down.

Richard.

Re: [PATCH] Remove special streaming of builtins

[Ilya Enkovich]

2016-07-26 22:52 GMT+03:00 Richard Biener :
> On July 26, 2016 7:26:46 PM GMT+02:00, "H.J. Lu"  wrote:
>>On Mon, Jul 25, 2016 at 4:35 AM, Richard Biener 
>>wrote:
>>>
>>> So I needed to fix that builtins appearing in BLOCK_VARs and the
>>solution
>>> I came up with accidentially disabled streaming via the special path.
>>> Thus the following patch removes the special-casing completely and
>>makes
>>> the BLOCK_VARs handling work the same way as for regular externs (by
>>> streaming a local copy).  We stream each builtin decl once and then
>>> refer to it via the decl index (which is cheaper than the special
>>> casing).
>>>
>>> I'm not 100% this solves for example the -fno-math-errno inlining
>>> across TUs (it certainly doesn't if you use attribute optimize with
>>> -fno-math-errno), but it eventually should by means of having two
>>> different BUILT_IN_XXX if they have different behavior.  At least
>>> if all relevant bits are set on the function _type_ rather than
>>> the decl which I think we still lto-symtab replace with one
>>> entity during WPA(?)
>>>
>>> Well.
>>>
>>> LTO bootstrapped and tested on x86_64-unknown-linux-gnu
>>(c,c++,fortran),
>>> bootstrapped on x86_64-unknown-linux-gnu (all), testing in progress.
>>>
>>> I might have not catched all fndecl compares.
>>>
>>> Will apply to trunk if testing completes.  As said, maybe followup
>>> cleanups possible, at least to lto-opts.c / lto-wrapper.
>>>
>>> Richard.
>>>
>>> 2016-07-25  Richard Biener  
>>>
>>> * cgraph.c (cgraph_node::verify_node): Compare against
>>builtin
>>> by using DECL_BUILT_IN_CLASS and DECL_FUNCTION_CODE.
>>> * tree-chkp.c (chkp_gimple_call_builtin_p): Likewise.
>>> * tree-streamer.h (streamer_handle_as_builtin_p): Remove.
>>> (streamer_get_builtin_tree): Likewise.
>>> (streamer_write_builtin): Likewise.
>>> * lto-streamer.h (LTO_builtin_decl): Remove.
>>> * lto-streamer-in.c (lto_read_tree_1): Remove assert.
>>> (lto_input_scc): Remove LTO_builtin_decl handling.
>>> (lto_input_tree_1): Liekwise.
>>> * lto-streamer-out.c (lto_output_tree_1): Remove special
>>> handling of builtins.
>>> (DFS::DFS): Likewise.
>>> * tree-streamer-in.c (streamer_get_builtin_tree): Remove.
>>> * tree-streamer-out.c (pack_ts_function_decl_value_fields):
>>Remove
>>> assert.
>>> (streamer_write_builtin): Remove.
>>>
>>> lto/
>>> * lto.c (compare_tree_sccs_1): Remove
>>streamer_handle_as_builtin_p uses.
>>> (unify_scc): Likewise.
>>> (lto_read_decls): Likewise.
>>>
>>
>>This caused:
>>
>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72683
>
> Probably another by-decl built-in function compare in the mpx support.  I 
> have fixed the one that triggered on a not mpx capable machine.  Ilya 
> possibly knows where the other one(s) are lurking off his head?

I found a couple more of such comparisons.  Their replacement fixes
PR72683 and PR72657.

Thanks,
Ilya

>
> Richard.
>
>

Re: [PATCH] Remove special streaming of builtins

[Richard Biener]

On July 26, 2016 7:26:46 PM GMT+02:00, "H.J. Lu"  wrote:
>On Mon, Jul 25, 2016 at 4:35 AM, Richard Biener 
>wrote:
>>
>> So I needed to fix that builtins appearing in BLOCK_VARs and the
>solution
>> I came up with accidentially disabled streaming via the special path.
>> Thus the following patch removes the special-casing completely and
>makes
>> the BLOCK_VARs handling work the same way as for regular externs (by
>> streaming a local copy).  We stream each builtin decl once and then
>> refer to it via the decl index (which is cheaper than the special
>> casing).
>>
>> I'm not 100% this solves for example the -fno-math-errno inlining
>> across TUs (it certainly doesn't if you use attribute optimize with
>> -fno-math-errno), but it eventually should by means of having two
>> different BUILT_IN_XXX if they have different behavior.  At least
>> if all relevant bits are set on the function _type_ rather than
>> the decl which I think we still lto-symtab replace with one
>> entity during WPA(?)
>>
>> Well.
>>
>> LTO bootstrapped and tested on x86_64-unknown-linux-gnu
>(c,c++,fortran),
>> bootstrapped on x86_64-unknown-linux-gnu (all), testing in progress.
>>
>> I might have not catched all fndecl compares.
>>
>> Will apply to trunk if testing completes.  As said, maybe followup
>> cleanups possible, at least to lto-opts.c / lto-wrapper.
>>
>> Richard.
>>
>> 2016-07-25  Richard Biener  
>>
>> * cgraph.c (cgraph_node::verify_node): Compare against
>builtin
>> by using DECL_BUILT_IN_CLASS and DECL_FUNCTION_CODE.
>> * tree-chkp.c (chkp_gimple_call_builtin_p): Likewise.
>> * tree-streamer.h (streamer_handle_as_builtin_p): Remove.
>> (streamer_get_builtin_tree): Likewise.
>> (streamer_write_builtin): Likewise.
>> * lto-streamer.h (LTO_builtin_decl): Remove.
>> * lto-streamer-in.c (lto_read_tree_1): Remove assert.
>> (lto_input_scc): Remove LTO_builtin_decl handling.
>> (lto_input_tree_1): Liekwise.
>> * lto-streamer-out.c (lto_output_tree_1): Remove special
>> handling of builtins.
>> (DFS::DFS): Likewise.
>> * tree-streamer-in.c (streamer_get_builtin_tree): Remove.
>> * tree-streamer-out.c (pack_ts_function_decl_value_fields):
>Remove
>> assert.
>> (streamer_write_builtin): Remove.
>>
>> lto/
>> * lto.c (compare_tree_sccs_1): Remove
>streamer_handle_as_builtin_p uses.
>> (unify_scc): Likewise.
>> (lto_read_decls): Likewise.
>>
>
>This caused:
>
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72683

Probably another by-decl built-in function compare in the mpx support.  I have 
fixed the one that triggered on a not mpx capable machine.  Ilya possibly knows 
where the other one(s) are lurking off his head?

Richard.

Re: [PATCH] Remove special streaming of builtins

[H.J. Lu]

On Mon, Jul 25, 2016 at 4:35 AM, Richard Biener  wrote:
>
> So I needed to fix that builtins appearing in BLOCK_VARs and the solution
> I came up with accidentially disabled streaming via the special path.
> Thus the following patch removes the special-casing completely and makes
> the BLOCK_VARs handling work the same way as for regular externs (by
> streaming a local copy).  We stream each builtin decl once and then
> refer to it via the decl index (which is cheaper than the special
> casing).
>
> I'm not 100% this solves for example the -fno-math-errno inlining
> across TUs (it certainly doesn't if you use attribute optimize with
> -fno-math-errno), but it eventually should by means of having two
> different BUILT_IN_XXX if they have different behavior.  At least
> if all relevant bits are set on the function _type_ rather than
> the decl which I think we still lto-symtab replace with one
> entity during WPA(?)
>
> Well.
>
> LTO bootstrapped and tested on x86_64-unknown-linux-gnu (c,c++,fortran),
> bootstrapped on x86_64-unknown-linux-gnu (all), testing in progress.
>
> I might have not catched all fndecl compares.
>
> Will apply to trunk if testing completes.  As said, maybe followup
> cleanups possible, at least to lto-opts.c / lto-wrapper.
>
> Richard.
>
> 2016-07-25  Richard Biener  
>
> * cgraph.c (cgraph_node::verify_node): Compare against builtin
> by using DECL_BUILT_IN_CLASS and DECL_FUNCTION_CODE.
> * tree-chkp.c (chkp_gimple_call_builtin_p): Likewise.
> * tree-streamer.h (streamer_handle_as_builtin_p): Remove.
> (streamer_get_builtin_tree): Likewise.
> (streamer_write_builtin): Likewise.
> * lto-streamer.h (LTO_builtin_decl): Remove.
> * lto-streamer-in.c (lto_read_tree_1): Remove assert.
> (lto_input_scc): Remove LTO_builtin_decl handling.
> (lto_input_tree_1): Liekwise.
> * lto-streamer-out.c (lto_output_tree_1): Remove special
> handling of builtins.
> (DFS::DFS): Likewise.
> * tree-streamer-in.c (streamer_get_builtin_tree): Remove.
> * tree-streamer-out.c (pack_ts_function_decl_value_fields): Remove
> assert.
> (streamer_write_builtin): Remove.
>
> lto/
> * lto.c (compare_tree_sccs_1): Remove streamer_handle_as_builtin_p 
> uses.
> (unify_scc): Likewise.
> (lto_read_decls): Likewise.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72683


-- 
H.J.

[PATCH] Remove special streaming of builtins

[Richard Biener]

So I needed to fix that builtins appearing in BLOCK_VARs and the solution
I came up with accidentially disabled streaming via the special path.
Thus the following patch removes the special-casing completely and makes
the BLOCK_VARs handling work the same way as for regular externs (by
streaming a local copy).  We stream each builtin decl once and then
refer to it via the decl index (which is cheaper than the special
casing).

I'm not 100% this solves for example the -fno-math-errno inlining
across TUs (it certainly doesn't if you use attribute optimize with
-fno-math-errno), but it eventually should by means of having two
different BUILT_IN_XXX if they have different behavior.  At least
if all relevant bits are set on the function _type_ rather than
the decl which I think we still lto-symtab replace with one
entity during WPA(?)

Well.

LTO bootstrapped and tested on x86_64-unknown-linux-gnu (c,c++,fortran),
bootstrapped on x86_64-unknown-linux-gnu (all), testing in progress.

I might have not catched all fndecl compares.

Will apply to trunk if testing completes.  As said, maybe followup
cleanups possible, at least to lto-opts.c / lto-wrapper.

Richard.

2016-07-25  Richard Biener  

* cgraph.c (cgraph_node::verify_node): Compare against builtin
by using DECL_BUILT_IN_CLASS and DECL_FUNCTION_CODE.
* tree-chkp.c (chkp_gimple_call_builtin_p): Likewise.
* tree-streamer.h (streamer_handle_as_builtin_p): Remove.
(streamer_get_builtin_tree): Likewise.
(streamer_write_builtin): Likewise.
* lto-streamer.h (LTO_builtin_decl): Remove.
* lto-streamer-in.c (lto_read_tree_1): Remove assert.
(lto_input_scc): Remove LTO_builtin_decl handling.
(lto_input_tree_1): Liekwise.
* lto-streamer-out.c (lto_output_tree_1): Remove special
handling of builtins.
(DFS::DFS): Likewise.
* tree-streamer-in.c (streamer_get_builtin_tree): Remove.
* tree-streamer-out.c (pack_ts_function_decl_value_fields): Remove
assert.
(streamer_write_builtin): Remove.

lto/
* lto.c (compare_tree_sccs_1): Remove streamer_handle_as_builtin_p uses.
(unify_scc): Likewise.
(lto_read_decls): Likewise.

Index: gcc/cgraph.c
===
--- gcc/cgraph.c(revision 238590)
+++ gcc/cgraph.c(working copy)
@@ -3136,8 +3136,9 @@ cgraph_node::verify_node (void)
  && !e->speculative
  /* Optimized out calls are redirected to __builtin_unreachable.  */
  && (e->frequency
- || e->callee->decl
-!= builtin_decl_implicit (BUILT_IN_UNREACHABLE))
+ || ! e->callee->decl
+ || DECL_BUILT_IN_CLASS (e->callee->decl) != BUILT_IN_NORMAL
+ || DECL_FUNCTION_CODE (e->callee->decl) != BUILT_IN_UNREACHABLE)
  && (e->frequency
  != compute_call_stmt_bb_frequency (e->caller->decl,
 gimple_bb (e->call_stmt
Index: gcc/lto/lto.c
===
--- gcc/lto/lto.c   (revision 238590)
+++ gcc/lto/lto.c   (working copy)
@@ -1061,12 +1061,6 @@ compare_tree_sccs_1 (tree t1, tree t2, t
TREE_FIXED_CST_PTR (t1), TREE_FIXED_CST_PTR (t2)))
   return false;
 
-
-  /* We want to compare locations up to the point where it makes
- a difference for streaming - thus whether the decl is builtin or not.  */
-  if (CODE_CONTAINS_STRUCT (code, TS_DECL_MINIMAL))
-compare_values (streamer_handle_as_builtin_p);
-
   if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
 {
   compare_values (DECL_MODE);
@@ -1602,8 +1596,7 @@ unify_scc (struct data_in *data_in, unsi
   streamer.  The others should be singletons, too, and we
   should not merge them in any way.  */
gcc_assert (code != TRANSLATION_UNIT_DECL
-   && code != IDENTIFIER_NODE
-   && !streamer_handle_as_builtin_p (t));
+   && code != IDENTIFIER_NODE);
  }
 
  /* Fixup the streamer cache with the prevailing nodes according
@@ -1710,8 +1703,7 @@ lto_read_decls (struct lto_file_decl_dat
  if (len == 1
  && (TREE_CODE (first) == IDENTIFIER_NODE
  || TREE_CODE (first) == INTEGER_CST
- || TREE_CODE (first) == TRANSLATION_UNIT_DECL
- || streamer_handle_as_builtin_p (first)))
+ || TREE_CODE (first) == TRANSLATION_UNIT_DECL))
continue;
 
  /* Try to unify the SCC with already existing ones.  */
Index: gcc/lto-streamer-in.c
===
--- gcc/lto-streamer-in.c   (revision 238590)
+++ gcc/lto-streamer-in.c   (working copy)
@@ -1302,10 +1302,6 @@