[UPC 02/22] tree-related changes

[Richard Biener]

On Wed, 2 Dec 2015, Gary Funck wrote:

> On 12/01/15 12:26:32, Richard Biener wrote:
> > On Mon, 30 Nov 2015, Gary Funck wrote:
> > > -struct GTY(()) tree_type_common {
> > > +struct GTY((user)) tree_type_common {
> > >    struct tree_common common;
> > >    tree size;
> > >    tree size_unit;
> > > @@ -1441,10 +1458,10 @@ struct GTY(()) tree_type_common {
> > >    tree pointer_to;
> > >    tree reference_to;
> > >    union tree_type_symtab {
> > > -    int GTY ((tag ("TYPE_SYMTAB_IS_ADDRESS"))) address;
> > > -    const char * GTY ((tag ("TYPE_SYMTAB_IS_POINTER"))) pointer;
> > > -    struct die_struct * GTY ((tag ("TYPE_SYMTAB_IS_DIE"))) die;
> > > -  } GTY ((desc ("debug_hooks->tree_type_symtab_field"))) symtab;
> > > +    int address;
> > > +    const char *pointer;
> > > +    struct die_struct *die;
> > > +  } symtab;
> >
> > Err, you don't have debug info for this?  What is address?
> 
> Not sure what you mean.  The 'die' field is retained.
> Is there something in the semantics of "GTY(( ((tag "
> that relates to debug information?

Ah, sorry.  I misread the diff.

> > I do not like the explict GC of tree_type_common.
> 
> I'm not a fan either.
> 
> The gist is that we needed a map from tree nodes to tree nodes
> to record the "layout qualifier" for layout qualifiers with
> a value greater than one.  But when the garbage collector ran
> over the hash table that maps integer constants to tree nodes,
> it didn't know that the constant was being referenced by the
> layout qualifier tree map.
> 
> We described the issue here:
> https://gcc.gnu.org/ml/gcc-patches/2011-10/msg00800.html
> 
> The conclusion that we reached is that when tree nodes
> were walked, we needed to check if there was a
> tree node -> integer constant mapping, the integer constant map
> (used to make tree nodes used to hold CST's unique)
> needed to be marked to keep the CST mapping from going away.
> 
> This led to the conclusion that a custom GC routine was
> needed for tree nodes.  Maybe that conclusion is wrong or
> there is a better way to do things?

It should simply work as long as the hash-map is properly marked
as GC root.  It might _not_ work (reliably) if the hash-map is
also a "cache" by itself.  But it eventually works now given some
fixes went into the area of collecting/marking caches.

> > > ===================================================================
> > > --- gcc/tree-pretty-print.c	(.../trunk)	(revision 231059)
> > > +++ gcc/tree-pretty-print.c	(.../branches/gupc)	(revision 231080)
> > > @@ -1105,6 +1105,25 @@ dump_block_node (pretty_printer *pp, tre
> > >  }
> > >  
> > >  
> > > +static void
> > > +dump_upc_type_quals (pretty_printer *buffer, tree type, int quals)
> >
> > Functions need comments.
> 
> OK.  Missed that one.  Will check on others.
> 
> > > Index: gcc/tree-sra.c
> > > ===================================================================
> > > --- gcc/tree-sra.c	(.../trunk)	(revision 231059)
> > > +++ gcc/tree-sra.c	(.../branches/gupc)	(revision 231080)
> > > @@ -3882,6 +3882,7 @@ find_param_candidates (void)
> > >  
> > >  	  if (TREE_CODE (type) == FUNCTION_TYPE
> > >  	      || TYPE_VOLATILE (type)
> > > +	      || SHARED_TYPE_P (type)
> > 
> > UPC_SHARED_TYPE_P ()
> 
> OK. As I mentioned in a previous reply, originally we prefixed
> all "UPC" specific tree node fields and functions with UPC_ or upc_,
> but as we transitioned away from UPC as a separate language
> (ala ObjC) and made compilation conditional upon -fupc, an
> observation was made off list that since the base tree nodes
> are generic that naming UPC-related fields with "UPC" prefixes
> didn't make sense, so we removed those prefixes.  There might
> be a middle ground, however, whee UPC_SHARED_TYPE_P() is preferred
> to SHARED_TYPE_P() because as you/others have mentioned,
> the term "shared" gets used in a lot of contexts.

Yes, specifically for predicates/functions used in the middle-end.

> > > @@ -4381,6 +4422,7 @@ build1_stat (enum tree_code code, tree t
> > >        /* Whether a dereference is readonly has nothing to do with whether
> > >  	 its operand is readonly.  */
> > >        TREE_READONLY (t) = 0;
> > > +      TREE_SHARED (t) = SHARED_TYPE_P (type);
> > 
> > This is frontend logic and should reside in FEs.
> 
> [... several other similar actions taken contingent
> upon SHARED_TYPE_P() elided ...]
> 
> OK, will take a look.
> 
> > > +  outer_is_pts_p = (POINTER_TYPE_P (outer_type)
> > > +                    && SHARED_TYPE_P (TREE_TYPE (outer_type)));
> > > +  inner_is_pts_p = (POINTER_TYPE_P (inner_type)
> > > +                    && SHARED_TYPE_P (TREE_TYPE (inner_type)));
> > > +
> > > +  /* Pointer-to-shared types have special
> > > +     equivalence rules that must be checked.  */
> > > +  if (outer_is_pts_p && inner_is_pts_p
> > > +      && lang_hooks.types_compatible_p)
> > > +    return lang_hooks.types_compatible_p (outer_type, inner_type);
> > 
> > Sorry, but that can't fly with LTO.
> 
> Can you elaborate?  I'm not familiar with LTO or its requirements.

With LTO the langhooks from the frontends are no longer available
so you'd need to implement the same logic in the LTO "frontend"
which means it wouldn't need a langhook at all because all information
would be contained in the middle-end representations.

> > I wonder why you didn't use address-spaces for whatever
> > UPC needs for pointers.
> 
> Background: UPC pointers have special language defined rules
> for address arithmetic.  In the simplest case, adding 1 to a UPC
> pointer-to-shared increments the thread part of the pointer, until
> it exceeds the number of threads in the program, at that point,
> the "virtual address" (often an offset into the program's global
> shared region) is incremented by the size of the element type.
> Since UPC pointers have a thread field and block offset (phase)
> field, they're 2x the size of a regular "C" pointer.
> 
> When we started working on UPC, GCC didn't have address space support,
> and from what I can tell from watching commits,
> address space support has improved over time.
> 
> Is it the case now that I could define a pointer to a special
> (reserved for UPC) address space that can have a mode that is, say,
> 2x the size of a regular pointer mode?  That would help.
> That would help in the cases where we need the compiler to
> use a different underlying mode for UPC pointers-to-shared.
> 
> We would also still need to make sure that the parts of the
> compiler that think that pointers are interchangeable with
> integers doesn't do that with UPC pointers-to-shared.

I'm not very familiar with address-spaces but at least they
can be used to distinguish different pointer kinds (and prevent
conversions).  Not sure if it would allow different behavior
for a simple plus.

Note that I belive you should lower your "pointer" representation
when you lower the rest (I suppose you lower this now only during
RTL generation?)

> > > +/* Return a variant of TYPE, where the shared, strict, and relaxed
> > > +   qualifiers have been removed.  */
> > > +
> > > +tree
> > > +build_unshared_type (tree type)
> > 
> > Too generic names.  I'd prefer if these routines were in a upc.c
> > file rather than in tree.c.  Likewise for the block hash I suppose.
> 
> As discussed previously, we used to prefix all UPC-related functions
> and types with UPC_ or upc_.  It would be easy to bring that back.
> 
> However, not sure how that squares with the off-list feedback
> that we received: If we add fields to the base tree type,
> they should not have language specific names in them.
> 
> > > +/* Set the block factor in the type described by NODE.
> > > +   For a zero blocking factor set TYPE_BLOCK_FACTOR_0 (NODE). 
> > > +   For a blocking factor greater than 1, insert the value
> > > +   into a hash table indexed by NODE, and then set the
> > > +   flag TYPE_BLOCK_FACTOR_X (NODE).  */
> > > +#define SET_TYPE_BLOCK_FACTOR(NODE, VAL) \
> > > [...]
> > 
> > Use a function please.  No such unwiedlingly large macros.
> 
> OK.  Are static inline's OK here?

Implementations in non-headers are prefered, but yes.

> > > +/* UPC pointer to shared qualified object representation */
> > > +#define upc_pts_rep_type_node	global_trees[TI_UPC_PTS_REP_TYPE]
> > > +#define upc_char_pts_type_node	global_trees[TI_UPC_CHAR_PTS_TYPE]
> > > +#define upc_phase_field_node	global_trees[TI_UPC_PHASE_FIELD]
> > > +#define upc_thread_field_node	global_trees[TI_UPC_THREAD_FIELD]
> > > +#define upc_vaddr_field_node	global_trees[TI_UPC_VADDR_FIELD]
> > > +#define upc_null_pts_node	global_trees[TI_UPC_NULL_PTS]
> > > +
> > 
> > Ugh.  Please no further global tree nodes.
> > Do you really need all of them in middle-end code?
> 
> Apart from tree.[ch], here is where those fields are referenced.
> 
> c/c-parser.c
> c/c-typeck.c
> c/c-upc.c
> c/c-upc-lang.c
> c/c-upc-low.c
> c/c-upc-pts-ops.c
> c-family/c-common.c
> config/i386/i386.c
> config/rs6000/rs6000.c
> convert.c
> function.c
> tree-pretty-print.c
> 
> Where do you draw the line between front-end and middle-end?
> Is everything after c_genericize() considered middle-end?

After gimplification.  I realize the global trees are in a messy
state already and most of them should be moved to frontend specific
places.  It would be a start if you do that.  And lowering your
pointer representation earlier will probably make that easier.

Richard.

> We definitely need those global tree nodes up through upc_genericize(),
> which is called just before c_genericize().  We might be able to
> do away with upc_phase_field_node, upc_thread_field_node,
> and upc_vaddr_field_node and find them in upc_pts_rep_type_node,
> which is struct with those fields.
> 
> In config/i386/i386.c, we only need the mode of the internal
> representation of a UPC pointer-to-shared type.
> 
>   if (type != NULL_TREE && POINTER_TYPE_P (type))
>     {
>       if (SHARED_TYPE_P (TREE_TYPE (type)))
>         {
>           *punsignedp = 1;
>           return TYPE_MODE (upc_pts_rep_type_node);
>         }
>       *punsignedp = POINTERS_EXTEND_UNSIGNED;
>       return word_mode;
>     }
> 
> In config/rs6000/rs6000.c, we need to know if the UPC pointer-to-shared
> internal representation is an aggregate type.
> 
> 
>   /* TRUE if TYPE is a UPC pointer-to-shared type
>      and its underlying representation is an aggregate.  */
>   bool upc_struct_pts_p = (POINTER_TYPE_P (type)
>                              && SHARED_TYPE_P (TREE_TYPE (type)))
>                            && AGGREGATE_TYPE_P (upc_pts_rep_type_node);
>   /* If TYPE is a UPC struct PTS type, handle it as an aggregate type.  */
>   bool aggregate_p = AGGREGATE_TYPE_P (type)
>                      || upc_struct_pts_p;
> 
> Until recently, we supported two internal representation types,
> packed and struct.  The packed representation was an int64
> but couldn't express the full range for threads, virtual address,
> and block offset.  So we decided to go only with 'struct'.
> That makes the test for AGGREGATE_TYPE_P (upc_pts_rep_type_node)
> redundant above, but left it there because we felt that it had
> some descriptive value.
> 
> Looks like both of those target dependent references to
> upc_pts_rep_type_node could go away if they need to.