On Mon, Nov 30, 2015 at 11:29:34PM +0300, Ilya Verbin wrote: > > This looks wrong, both of these clearly could affect anything with > > DECL_HAS_VALUE_EXPR_P, not just the link vars. > > So, if you need to handle the "omp declare target link" vars specially, > > you should only handle those specially and nothing else. And please try to > > explain why. > > Actually these ifndefs are not needed, because assemble_decl never will be > called by accel compiler for original link vars. I've added a check into > output_in_order, but missed a second place where assemble_decl is called - > symbol_table::output_variables. So, fixed now.
Great. > > Do we need to do anything in gomp_unload_image_from_device ? > > I mean at least in questionable programs that for link vars don't decrement > > the refcount of the var that replaced the link var to 0 first before > > dlclosing the library. > > At least host_var_table[j * 2 + 1] will have the MSB set, so we need to > > handle it differently. Perhaps for that case perform a lookup, and if we > > get something which has link_map non-NULL, first perform as if there is > > target exit data delete (var) on it first? > > You're right, it doesn't deallocate memory on the device if DSO leaves nonzero > refcount. And currently host compiler doesn't set MSB in host_var_table, it's > set only by accel compiler. But it's possible to do splay_tree_lookup for > each > var to determine whether is it linked or not, like in the patch bellow. > Or do you prefer to set the bit in host compiler too? It requires > lookup_attribute ("omp declare target link") for all vars in the table during > compilation, but allows to do splay_tree_lookup at run-time only for vars with > MSB set in host_var_table. > Unfortunately, calling gomp_exit_data from gomp_unload_image_from_device works > only for DSO, but it crashed when an executable leaves nonzero refcount, > because > target device may be already uninitialized from plugin's __run_exit_handlers > (and it is in case of intelmic), so gomp_exit_data cannot run free_func. > Is it possible do add some atexit (...) to libgomp, which will set > shutting_down > flag, and just do nothing in gomp_unload_image_from_device if it is set? Sorry, I didn't mean you should call gomp_exit_data, what I meant was that you perform the same action as would delete(var) do in that case. Calling gomp_exit_data e.g. looks it up again etc. Supposedly having the MSB in host table too is useful, so if you could handle that, it would be nice. And splay_tree_lookup only if the MSB is set. So, if (!host_data_has_msb_set) splay_tree_remove (&devicep->mem_map, &k); else { splay_tree_key n = splay_tree_lookup (&devicep->mem_map, &k); if (n->link_key) { n->refcount = 0; n->link_key = NULL; splay_tree_remove (&devicep->mem_map, n); if (n->tgt->refcount > 1) n->tgt->refcount--; else gomp_unmap_tgt (n->tgt); } else splay_tree_remove (&devicep->mem_map, n); } or so. Jakub