Re: [PATCH 9c] callgraph: handle __RTL functions

[Jan Hubicka]

> 
> They're added to the cgraph by this call:
> 
>   /* Add to cgraph.  */
>   cgraph_node::finalize_function (fndecl, false);
> 
> within function_reader::create_function (in r244110, though that code
> isn't called yet; it's called by the stuff in patch 9).
> 
> If I hack out that call, so that __RTL functions aren't in the cgraph,
> then I see lots of failures in the kit, for example here in predict.c:
> 
> maybe_hot_frequency_p (struct function *fun, int freq)
> {
>   struct cgraph_node *node = cgraph_node::get (fun->decl);
> 
>   [...read though node, so it must be non-NULL]
> 
> Similarly, this line in varasm.c's assemble_start_function assumes that
> the fndecl has a symtab node:
> 
>   align = symtab_node::get (decl)->definition_alignment ();
> 
> etc.
> 
> I don't know how many other places make the assumption that cfun's
> fndecl has a node in the callgraph.
> 
> Given that I want to have __RTL functions called by non-__RTL functions
> (and the patch kit handles this), it seemed saner to go down the route
> of adding the decl to the callgraph.

You need to have symbol in the symbol table, but you don't necessarily need to
finalize it.  You can do cgraph_node::get_cteate instead of finalize_function.
finalize_function is meant as an api for frontend saying "I am done working on
this definition and I want to pass it to middle-end", while the RTL front-end
at the moment does everything by itself (I must say I am bit worried about
consequences of this because it gets us back from unit-at-a-time to
sort-of-unit-at-a-time which used to be quite some pain, but we can work them
out incrementally).

RTL backend may at some point assert that callgraph node of function being
compiled is an analyzed definition.

I guess the patchset is fine as it is and I will clean it up next stage1.
It definitly adds less clutter than chkp.
Honza

Re: [PATCH 9c] callgraph: handle __RTL functions

[Jeff Law]

On 01/17/2017 02:21 AM, Richard Biener wrote:


So I guess my question is how do you ensure that even though cgraph hasn't
looked at code that we're appropriately conservative with how the file is
processed?  Particularly if there's other code in the source file that is
expected to interact with the RTL native code?


I think that as we're finalizing the function from the FE before the
cgraph is built
(and even throw away the RTL?) we have no other choice than treating a __RTL
function as black box which means treat it as possibly calling all function in
the TU and reading/writing/taking the address of all decls in the TU.  Consider

static int i;
static void foo () {}
int __RTL main()
{
  ... call foo, access i ...
}

which probably will right now optimize i and foo away and thus fail to link?
That's what I think will currently happen.  I don't know the IPA bits 
very well, so I could have missed something.





But I think we can sort out these "details" when we run into them...
I can live with that -- I strongly suspect we're going to find all kinds 
of things of a similar nature the more we poke at this stuff.


With that in mind, I'll go ahead and ack 9c for the trunk with the 
explicit understanding that we know there's stuff that's going to break 
the harder we push it and we'll incrementally work to improve it.


It certainly helps that I see this as strictly for developers, not 
users.  So I'm willing to be more lax on a lot of stuff.


jeff

Re: [PATCH 9c] callgraph: handle __RTL functions

[David Malcolm]

On Tue, 2017-01-17 at 13:35 +0100, Jan Hubicka wrote:
> > On Mon, Jan 16, 2017 at 10:25 PM, Jeff Law  wrote:
> > > On 01/09/2017 07:38 PM, David Malcolm wrote:
> > > > 
> > > > The RTL backend code is full of singleton state, so we have to
> > > > handle
> > > > functions as soon as we parse them.  This requires various
> > > > special-casing
> > > > in the callgraph code.
> > > > 
> > > > gcc/ChangeLog:
> > > > * cgraph.h (symtab_node::native_rtl_p): New decl.
> > > > * cgraphunit.c (symtab_node::native_rtl_p): New
> > > > function.
> > > > (symtab_node::needed_p): Don't assert for early
> > > > assembly output
> > > > for __RTL functions.
> > > > (cgraph_node::finalize_function): Set "force_output"
> > > > for __RTL
> > > > functions.
> > > > (cgraph_node::analyze): Bail out early for __RTL
> > > > functions.
> > > > (analyze_functions): Update assertion to support __RTL
> > > > functions.
> > > > (cgraph_node::expand): Bail out early for __RTL
> > > > functions.
> > > > * gimple-expr.c: Include "tree-pass.h".
> > > > (gimple_has_body_p): Return false for __RTL functions.
> > > > ---
> > > >  gcc/cgraph.h  |  4 
> > > >  gcc/cgraphunit.c  | 41 ++-
> > > > --
> > > >  gcc/gimple-expr.c |  3 ++-
> > > >  3 files changed, 44 insertions(+), 4 deletions(-)
> > > > 
> > > 
> > > > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> > > > index 81a3ae9..ed699e1 100644
> > > > --- a/gcc/cgraphunit.c
> > > > +++ b/gcc/cgraphunit.c
> > > 
> > >  @@ -568,6 +591,12 @@ cgraph_node::add_new_function (tree fndecl,
> > > bool
> > > lowered)
> > > > 
> > > >  void
> > > >  cgraph_node::analyze (void)
> > > >  {
> > > > +  if (native_rtl_p ())
> > > > +{
> > > > +  analyzed = true;
> > > > +  return;
> > > > +}
> > > 
> > > So my concern here would be how this interacts with the rest of
> > > the cgraph
> > > machinery.  Essentially you're saying we've built all the
> > > properties for the
> > > given code.  But AFAICT that can't be true and cgraph isn't
> > > actually aware
> > > of any of the properties of the native RTL code (even such things
> > > as what
> > > functions the native RTL code might call).
> > > 
> > > So I guess my question is how do you ensure that even though
> > > cgraph hasn't
> > > looked at code that we're appropriately conservative with how the
> > > file is
> > > processed?  Particularly if there's other code in the source file
> > > that is
> > > expected to interact with the RTL native code?
> > 
> > I think that as we're finalizing the function from the FE before
> > the
> > cgraph is built
> > (and even throw away the RTL?) we have no other choice than
> > treating a __RTL
> > function as black box which means treat it as possibly calling all
> > function in
> > the TU and reading/writing/taking the address of all decls in the
> > TU.  Consider
> 
> I guess RTL frontend may be arranged to mark all such decls as used
> or just require
> user to do it, like we do with asm statements.
> 
> I wonder why we need to insert those definitions into cgraph at first
> place...

They're added to the cgraph by this call:

  /* Add to cgraph.  */
  cgraph_node::finalize_function (fndecl, false);

within function_reader::create_function (in r244110, though that code
isn't called yet; it's called by the stuff in patch 9).

If I hack out that call, so that __RTL functions aren't in the cgraph,
then I see lots of failures in the kit, for example here in predict.c:

maybe_hot_frequency_p (struct function *fun, int freq)
{
  struct cgraph_node *node = cgraph_node::get (fun->decl);

  [...read though node, so it must be non-NULL]

Similarly, this line in varasm.c's assemble_start_function assumes that
the fndecl has a symtab node:

  align = symtab_node::get (decl)->definition_alignment ();

etc.

I don't know how many other places make the assumption that cfun's
fndecl has a node in the callgraph.

Given that I want to have __RTL functions called by non-__RTL functions
(and the patch kit handles this), it seemed saner to go down the route
of adding the decl to the callgraph.

> Honza
> > 
> > static int i;
> > static void foo () {}
> > int __RTL main()
> > {
> >   ... call foo, access i ...
> > }
> > 
> > which probably will right now optimize i and foo away and thus fail
> > to link?

> > But I think we can sort out these "details" when we run into
> > them...
> > 
> > Richard.
> > 
> > > Jeff

Re: [PATCH 9c] callgraph: handle __RTL functions

[Jan Hubicka]

> On Mon, Jan 16, 2017 at 10:25 PM, Jeff Law  wrote:
> > On 01/09/2017 07:38 PM, David Malcolm wrote:
> >>
> >> The RTL backend code is full of singleton state, so we have to handle
> >> functions as soon as we parse them.  This requires various special-casing
> >> in the callgraph code.
> >>
> >> gcc/ChangeLog:
> >> * cgraph.h (symtab_node::native_rtl_p): New decl.
> >> * cgraphunit.c (symtab_node::native_rtl_p): New function.
> >> (symtab_node::needed_p): Don't assert for early assembly output
> >> for __RTL functions.
> >> (cgraph_node::finalize_function): Set "force_output" for __RTL
> >> functions.
> >> (cgraph_node::analyze): Bail out early for __RTL functions.
> >> (analyze_functions): Update assertion to support __RTL functions.
> >> (cgraph_node::expand): Bail out early for __RTL functions.
> >> * gimple-expr.c: Include "tree-pass.h".
> >> (gimple_has_body_p): Return false for __RTL functions.
> >> ---
> >>  gcc/cgraph.h  |  4 
> >>  gcc/cgraphunit.c  | 41 ++---
> >>  gcc/gimple-expr.c |  3 ++-
> >>  3 files changed, 44 insertions(+), 4 deletions(-)
> >>
> >
> >> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> >> index 81a3ae9..ed699e1 100644
> >> --- a/gcc/cgraphunit.c
> >> +++ b/gcc/cgraphunit.c
> >
> >  @@ -568,6 +591,12 @@ cgraph_node::add_new_function (tree fndecl, bool
> > lowered)
> >>
> >>  void
> >>  cgraph_node::analyze (void)
> >>  {
> >> +  if (native_rtl_p ())
> >> +{
> >> +  analyzed = true;
> >> +  return;
> >> +}
> >
> > So my concern here would be how this interacts with the rest of the cgraph
> > machinery.  Essentially you're saying we've built all the properties for the
> > given code.  But AFAICT that can't be true and cgraph isn't actually aware
> > of any of the properties of the native RTL code (even such things as what
> > functions the native RTL code might call).
> >
> > So I guess my question is how do you ensure that even though cgraph hasn't
> > looked at code that we're appropriately conservative with how the file is
> > processed?  Particularly if there's other code in the source file that is
> > expected to interact with the RTL native code?
> 
> I think that as we're finalizing the function from the FE before the
> cgraph is built
> (and even throw away the RTL?) we have no other choice than treating a __RTL
> function as black box which means treat it as possibly calling all function in
> the TU and reading/writing/taking the address of all decls in the TU.  
> Consider

I guess RTL frontend may be arranged to mark all such decls as used or just 
require
user to do it, like we do with asm statements.

I wonder why we need to insert those definitions into cgraph at first place...

Honza
> 
> static int i;
> static void foo () {}
> int __RTL main()
> {
>   ... call foo, access i ...
> }
> 
> which probably will right now optimize i and foo away and thus fail to link?
> 
> But I think we can sort out these "details" when we run into them...
> 
> Richard.
> 
> > Jeff

Re: [PATCH 9c] callgraph: handle __RTL functions

[Richard Biener]

On Mon, Jan 16, 2017 at 10:25 PM, Jeff Law  wrote:
> On 01/09/2017 07:38 PM, David Malcolm wrote:
>>
>> The RTL backend code is full of singleton state, so we have to handle
>> functions as soon as we parse them.  This requires various special-casing
>> in the callgraph code.
>>
>> gcc/ChangeLog:
>> * cgraph.h (symtab_node::native_rtl_p): New decl.
>> * cgraphunit.c (symtab_node::native_rtl_p): New function.
>> (symtab_node::needed_p): Don't assert for early assembly output
>> for __RTL functions.
>> (cgraph_node::finalize_function): Set "force_output" for __RTL
>> functions.
>> (cgraph_node::analyze): Bail out early for __RTL functions.
>> (analyze_functions): Update assertion to support __RTL functions.
>> (cgraph_node::expand): Bail out early for __RTL functions.
>> * gimple-expr.c: Include "tree-pass.h".
>> (gimple_has_body_p): Return false for __RTL functions.
>> ---
>>  gcc/cgraph.h  |  4 
>>  gcc/cgraphunit.c  | 41 ++---
>>  gcc/gimple-expr.c |  3 ++-
>>  3 files changed, 44 insertions(+), 4 deletions(-)
>>
>
>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
>> index 81a3ae9..ed699e1 100644
>> --- a/gcc/cgraphunit.c
>> +++ b/gcc/cgraphunit.c
>
>  @@ -568,6 +591,12 @@ cgraph_node::add_new_function (tree fndecl, bool
> lowered)
>>
>>  void
>>  cgraph_node::analyze (void)
>>  {
>> +  if (native_rtl_p ())
>> +{
>> +  analyzed = true;
>> +  return;
>> +}
>
> So my concern here would be how this interacts with the rest of the cgraph
> machinery.  Essentially you're saying we've built all the properties for the
> given code.  But AFAICT that can't be true and cgraph isn't actually aware
> of any of the properties of the native RTL code (even such things as what
> functions the native RTL code might call).
>
> So I guess my question is how do you ensure that even though cgraph hasn't
> looked at code that we're appropriately conservative with how the file is
> processed?  Particularly if there's other code in the source file that is
> expected to interact with the RTL native code?

I think that as we're finalizing the function from the FE before the
cgraph is built
(and even throw away the RTL?) we have no other choice than treating a __RTL
function as black box which means treat it as possibly calling all function in
the TU and reading/writing/taking the address of all decls in the TU.  Consider

static int i;
static void foo () {}
int __RTL main()
{
  ... call foo, access i ...
}

which probably will right now optimize i and foo away and thus fail to link?

But I think we can sort out these "details" when we run into them...

Richard.

> Jeff

Re: [PATCH 9c] callgraph: handle __RTL functions

[Jeff Law]

On 01/09/2017 07:38 PM, David Malcolm wrote:

The RTL backend code is full of singleton state, so we have to handle
functions as soon as we parse them.  This requires various special-casing
in the callgraph code.

gcc/ChangeLog:
* cgraph.h (symtab_node::native_rtl_p): New decl.
* cgraphunit.c (symtab_node::native_rtl_p): New function.
(symtab_node::needed_p): Don't assert for early assembly output
for __RTL functions.
(cgraph_node::finalize_function): Set "force_output" for __RTL
functions.
(cgraph_node::analyze): Bail out early for __RTL functions.
(analyze_functions): Update assertion to support __RTL functions.
(cgraph_node::expand): Bail out early for __RTL functions.
* gimple-expr.c: Include "tree-pass.h".
(gimple_has_body_p): Return false for __RTL functions.
---
 gcc/cgraph.h  |  4 
 gcc/cgraphunit.c  | 41 ++---
 gcc/gimple-expr.c |  3 ++-
 3 files changed, 44 insertions(+), 4 deletions(-)




diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 81a3ae9..ed699e1 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
 @@ -568,6 +591,12 @@ cgraph_node::add_new_function (tree fndecl, bool 
lowered)

 void
 cgraph_node::analyze (void)
 {
+  if (native_rtl_p ())
+{
+  analyzed = true;
+  return;
+}
So my concern here would be how this interacts with the rest of the 
cgraph machinery.  Essentially you're saying we've built all the 
properties for the given code.  But AFAICT that can't be true and cgraph 
isn't actually aware of any of the properties of the native RTL code 
(even such things as what functions the native RTL code might call).


So I guess my question is how do you ensure that even though cgraph 
hasn't looked at code that we're appropriately conservative with how the 
file is processed?  Particularly if there's other code in the source 
file that is expected to interact with the RTL native code?


Jeff

[PATCH 9c] callgraph: handle __RTL functions

[David Malcolm]

The RTL backend code is full of singleton state, so we have to handle
functions as soon as we parse them.  This requires various special-casing
in the callgraph code.

gcc/ChangeLog:
* cgraph.h (symtab_node::native_rtl_p): New decl.
* cgraphunit.c (symtab_node::native_rtl_p): New function.
(symtab_node::needed_p): Don't assert for early assembly output
for __RTL functions.
(cgraph_node::finalize_function): Set "force_output" for __RTL
functions.
(cgraph_node::analyze): Bail out early for __RTL functions.
(analyze_functions): Update assertion to support __RTL functions.
(cgraph_node::expand): Bail out early for __RTL functions.
* gimple-expr.c: Include "tree-pass.h".
(gimple_has_body_p): Return false for __RTL functions.
---
 gcc/cgraph.h  |  4 
 gcc/cgraphunit.c  | 41 ++---
 gcc/gimple-expr.c |  3 ++-
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index db2915c..edaae51 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -328,6 +328,10 @@ public:
  configury. This function is used just during symbol creation.  */
   bool needed_p (void);
 
+  /* Return true if this symbol is a function from the C frontend specified
+ directly in RTL form (with "__RTL").  */
+  bool native_rtl_p () const;
+
   /* Return true when there are references to the node.  */
   bool referred_to_p (bool include_self = true);
 
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 81a3ae9..ed699e1 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -217,6 +217,19 @@ static void handle_alias_pairs (void);
 /* Used for vtable lookup in thunk adjusting.  */
 static GTY (()) tree vtable_entry_type;
 
+/* Return true if this symbol is a function from the C frontend specified
+   directly in RTL form (with "__RTL").  */
+
+bool
+symtab_node::native_rtl_p () const
+{
+  if (TREE_CODE (decl) != FUNCTION_DECL)
+return false;
+  if (!DECL_STRUCT_FUNCTION (decl))
+return false;
+  return DECL_STRUCT_FUNCTION (decl)->curr_properties & PROP_rtl;
+}
+
 /* Determine if symbol declaration is needed.  That is, visible to something
either outside this translation unit, something magic in the system
configury */
@@ -225,8 +238,10 @@ symtab_node::needed_p (void)
 {
   /* Double check that no one output the function into assembly file
  early.  */
-  gcc_checking_assert (!DECL_ASSEMBLER_NAME_SET_P (decl)
-  || !TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)));
+  if (!native_rtl_p ())
+  gcc_checking_assert
+   (!DECL_ASSEMBLER_NAME_SET_P (decl)
+|| !TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)));
 
   if (!definition)
 return false;
@@ -435,6 +450,14 @@ cgraph_node::finalize_function (tree decl, bool no_collect)
   && !DECL_DISREGARD_INLINE_LIMITS (decl))
 node->force_output = 1;
 
+  /* __RTL functions were already output as soon as they were parsed (due
+ to the large amount of global state in the backend).
+ Mark such functions as "force_output" to reflect the fact that they
+ will be in the asm file when considering the symbols they reference.
+ The attempt to output them later on will bail out immediately.  */
+  if (node->native_rtl_p ())
+node->force_output = 1;
+
   /* When not optimizing, also output the static functions. (see
  PR24561), but don't do so for always_inline functions, functions
  declared inline and nested functions.  These were optimized out
@@ -568,6 +591,12 @@ cgraph_node::add_new_function (tree fndecl, bool lowered)
 void
 cgraph_node::analyze (void)
 {
+  if (native_rtl_p ())
+{
+  analyzed = true;
+  return;
+}
+
   tree decl = this->decl;
   location_t saved_loc = input_location;
   input_location = DECL_SOURCE_LOCATION (decl);
@@ -1226,7 +1255,8 @@ analyze_functions (bool first_time)
 
  gcc_assert (!cnode->definition || cnode->thunk.thunk_p
  || cnode->alias
- || gimple_has_body_p (decl));
+ || gimple_has_body_p (decl)
+ || cnode->native_rtl_p ());
  gcc_assert (cnode->analyzed == cnode->definition);
}
   node->aux = NULL;
@@ -1965,6 +1995,11 @@ cgraph_node::expand (void)
   /* We ought to not compile any inline clones.  */
   gcc_assert (!global.inlined_to);
 
+  /* __RTL functions are compiled as soon as they are parsed, so don't
+ do it again.  */
+  if (native_rtl_p ())
+return;
+
   announce_function (decl);
   process = 0;
   gcc_assert (lowered);
diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
index b435b99..2ee87c2 100644
--- a/gcc/gimple-expr.c
+++ b/gcc/gimple-expr.c
@@ -34,6 +34,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "demangle.h"
 #include "hash-set.h"
 #include "rtl.h"
+#include "tree-pass.h"
 
 /* - Type related -  */
 
@@ -323,7 +324,7 @@ bool