Re: [gomp4.5] Handle #pragma omp declare target link

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Nov 16, 2015 at 06:40:43PM +0300, Ilya Verbin wrote:
> Here is WIP patch, not for check-in.  There are still many FIXMEs, which I am
> going to resolve, however target-link-1.c testcase pass.
> Is this approach correct?  Any comments on FIXMEs?
> 
> 
> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 23d0107..58771c0 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -15895,7 +15895,10 @@ c_parser_omp_declare_target (c_parser *parser)
>  	      g->have_offload = true;
>  	      if (is_a <varpool_node *> (node))
>  		{
> -		  vec_safe_push (offload_vars, t);
> +		  omp_offload_var var;
> +		  var.decl = t;
> +		  var.link_ptr_decl = NULL_TREE;
> +		  vec_safe_push (offload_vars, var);
>  		  node->force_output = 1;
>  		}

Another possible approach would be to keep offload_vars as
vector of trees, and simply push 2 trees in each case.
Or not to change this at all, see below.

> @@ -2009,7 +2010,8 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
>  	  decl = OMP_CLAUSE_DECL (c);
>  	  /* Global variables with "omp declare target" attribute
>  	     don't need to be copied, the receiver side will use them
> -	     directly.  */
> +	     directly.  However, global variables with "omp declare target link"
> +	     attribute need to be copied.  */
>  	  if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
>  	      && DECL_P (decl)
>  	      && ((OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_FIRSTPRIVATE_POINTER
> @@ -2017,7 +2019,9 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
>  		       != GOMP_MAP_FIRSTPRIVATE_REFERENCE))
>  		  || TREE_CODE (TREE_TYPE (decl)) == ARRAY_TYPE)
>  	      && is_global_var (maybe_lookup_decl_in_outer_ctx (decl, ctx))
> -	      && varpool_node::get_create (decl)->offloadable)
> +	      && varpool_node::get_create (decl)->offloadable
> +	      && !lookup_attribute ("omp declare target link",
> +				    DECL_ATTRIBUTES (decl)))

I wonder if Honza/Richi wouldn't prefer to have this info also
in cgraph, instead of looking up the attribute in each case.

> +      if (var.link_ptr_decl == NULL_TREE)
> +	addr = build_fold_addr_expr (var.decl);
> +      else
> +	{
> +	  /* For "omp declare target link" var use address of the pointer
> +	     instead of address of the var.  */
> +	  addr = build_fold_addr_expr (var.link_ptr_decl);
> +	  /* Most significant bit of the size marks such vars.  */
> +	  unsigned HOST_WIDE_INT isize = tree_to_uhwi (size);
> +	  isize |= 1ULL << (int_size_in_bytes (const_ptr_type_node) * 8 - 1);
> +	  size = wide_int_to_tree (const_ptr_type_node, isize);
> +
> +	  /* FIXME: Remove varpool node of var?  */

There is varpool_node::remove (), but not sure if at this point all the
references are already gone.

> +class pass_omp_target_link : public gimple_opt_pass
> +{
> +public:
> +  pass_omp_target_link (gcc::context *ctxt)
> +    : gimple_opt_pass (pass_data_omp_target_link, ctxt)
> +  {}
> +
> +  /* opt_pass methods: */
> +  virtual bool gate (function *fun)
> +    {
> +#ifdef ACCEL_COMPILER
> +      /* FIXME: Replace globals in target regions too or not?  */
> +      return lookup_attribute ("omp declare target",
> +			       DECL_ATTRIBUTES (fun->decl));

Certainly in "omp declare target entrypoint" regions too.

> +unsigned
> +pass_omp_target_link::execute (function *fun)
> +{
> +  basic_block bb;
> +  FOR_EACH_BB_FN (bb, fun)
> +    {
> +      gimple_stmt_iterator gsi;
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  unsigned i;
> +	  gimple *stmt = gsi_stmt (gsi);
> +	  for (i = 0; i < gimple_num_ops (stmt); i++)
> +	    {
> +	      tree op = gimple_op (stmt, i);
> +	      tree var = NULL_TREE;
> +
> +	      if (!op)
> +		continue;
> +	      if (TREE_CODE (op) == VAR_DECL)
> +		var = op;
> +	      else if (TREE_CODE (op) == ADDR_EXPR)
> +		{
> +		  tree op1 = TREE_OPERAND (op, 0);
> +		  if (TREE_CODE (op1) == VAR_DECL)
> +		    var = op1;
> +		}
> +	      /* FIXME: Support arrays.  What else?  */

We need to support all the references to the variables.
So, I think this approach is not right.

> +
> +	      if (var && lookup_attribute ("omp declare target link",
> +					   DECL_ATTRIBUTES (var)))
> +		{
> +		  tree type = TREE_TYPE (var);
> +		  tree ptype = build_pointer_type (type);
> +
> +		  /* Find var in offload table.  */
> +		  omp_offload_var *table_entry = NULL;
> +		  for (unsigned j = 0; j < vec_safe_length (offload_vars); j++)
> +		    if ((*offload_vars)[j].decl == var)
> +		      {
> +			table_entry = &(*offload_vars)[j];
> +			break;
> +		      }

Plus this would be terribly expensive if there are many variables in
offload_vars.
So, what I think should be done instead is that you first somewhere, perhaps
when streaming in the decls from LTO in ACCEL_COMPILER or so, create
the artificial link ptr variables for the "omp declare target link"
global vars and
  SET_DECL_VALUE_EXPR (var, build_simple_mem_ref (link_ptr_var));
  DECL_HAS_VALUE_EXPR_P (var) = 1;
and then in this pass just walk_gimple_stmt each stmt, with a
callback that would check for VAR_DECLs with DECL_HAS_VALUE_EXPR_P set
and in that case check if they are "omp declare target link", and if found
signal to the caller that the stmt needs to be regimplified, then just
gimple_regimplify_operands those stmts.

> +		  gcc_assert (table_entry);
> +
> +		  /* Get or create artificial pointer for the var.  */
> +		  tree ptr_decl;
> +		  if (table_entry->link_ptr_decl != NULL_TREE)
> +		    ptr_decl = table_entry->link_ptr_decl;
> +		  else
> +		    {
> +		      /* FIXME: Create a new node instead of copying?
> +			 Which info to preserve?  */
> +		      ptr_decl = copy_node (var);

I think you want a new node instead of copying.  You don't really want to
copy anything, perhaps TREE_USED, and set DECL_NAME to something derived
from the original name.  Make the ptr DECL_ARTIFICIAL and perhaps
DECL_NAMELESS.

> diff --git a/libgomp/target.c b/libgomp/target.c
> index ef22329..195be43 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -78,6 +78,17 @@ static int num_devices;
>  /* Number of GOMP_OFFLOAD_CAP_OPENMP_400 devices.  */
>  static int num_devices_openmp;
>  
> +/* FIXME: Quick and dirty prototype of keeping correspondence between host
> +   address of the object and target address of the artificial link pointer.
> +   Move it to gomp_device_descr, or where?  */
> +struct link_struct
> +{
> +  uintptr_t host_start;
> +  uintptr_t tgt_link_ptr;
> +};
> +static struct link_struct links[100];
> +static int link_num;

As for the representation, I think one possibility would be to say define
REFCOUNT_LINK (~(uintptr_t) 1)
and register at gomp_load_image_to_device time the link vars with that
refcount instead of REFCOUNT_INFINITY.  If k->refcount == REFCOUNT_LINK
then k->tgt_offset would be the pointer to the artificial pointer variable
instead of actual mapping; for say pointer lookup purposes
k->refcount == REFCOUNT_LINK would be treated as not mapped, and
gomp_map_vars if mapping something over that would simply temporarily
replace (remove the old splay tree key, add the new one) the REFCOUNT_LINK entry
with the new mapping (and store the pointer).  Then for the even when the
new mapping's refcount drops to zero we need to ensure that we readd the
REFCOUNT_LINK entry.  For that we need to store the old splay_tree_key
somewhere.  Either we can add it to splay_tree_key_s, but then it will be
around unconditionally for all entries, and splay_tree_node right now is
nicely power of 2-ish - 8 pointers.  Or stick it somewhere in
struct target_mem_desc, say splay_tree_key *link; and if the tgt has tgt->array
allocated and any of the mappings were previously REFCOUNT_LINK, then you could
either allocate that link array with not_found_cnt elements, or allocate
together with tgt->array and just point it after the last entry in
tgt->array.  tgt->link[i] would be NULL if tgt->array[i] splay_tree_node_s
did not replace REFCOUNT_LINK when created, and the old REFCOUNT_LINK entry
otherwise.  When do_unmap or exit_data, before splay_tree_remove you'd
find corresponding link entry (k should point to &k->tgt->array[X].key
for some X, so (splay_tree_node) k - k->tgt->array should be X and thus
splay_tree_key linkk = NULL;
if (k->tgt->link)
  linkk = k->tgt->link[(splay_tree_node) k - k->tgt->array];
before
  splay_tree_remove (&devicep->mem_map, k);
should hopefully give you the splay_tree_key to insert again.

	Jakub