| Message ID | 533D8894.4010706@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
2014-04-03 20:13 GMT+04:00 Bernd Schmidt <bernds@codesourcery.com>:
> The patch below should be a better fix, making the references to > __OPENMP_TARGET__ weak. Does this work for you?
Shouldn't we just remove __OPENMP_TARGET__ argument from GOMP_target,
since we decided to pass it to GOMP_offload_register?
-- Ilya
On 04/03/2014 06:53 PM, Ilya Verbin wrote: > 2014-04-03 20:13 GMT+04:00 Bernd Schmidt <bernds@codesourcery.com>: >> The patch below should be a better fix, making the references to > __OPENMP_TARGET__ weak. Does this work for you? > > Shouldn't we just remove __OPENMP_TARGET__ argument from GOMP_target, > since we decided to pass it to GOMP_offload_register? I thought it was used to look up the right function? With shared libraries you'd get multiple __OPENMP_TARGET__ tables. Bernd
2014-04-03 21:06 GMT+04:00 Bernd Schmidt <bernds@codesourcery.com>: > On 04/03/2014 06:53 PM, Ilya Verbin wrote: >> >> 2014-04-03 20:13 GMT+04:00 Bernd Schmidt <bernds@codesourcery.com>: >>> >>> The patch below should be a better fix, making the references to > >>> __OPENMP_TARGET__ weak. Does this work for you? >> >> >> Shouldn't we just remove __OPENMP_TARGET__ argument from GOMP_target, >> since we decided to pass it to GOMP_offload_register? > > > I thought it was used to look up the right function? With shared libraries > you'd get multiple __OPENMP_TARGET__ tables. > > > Bernd > Yes, initially the idea was to use it for look up the right function. But now each DSO will call GOMP_offload_register, and pass unique pointer to __OPENMP_TARGET__ (host_table) for this DSO. Then gomp_register_images_for_device registers all this host tables in the plugin. And when libgomp calls device_get_table_func, the plugin returns the joint table for all DSO's. -- Ilya
On 04/03/2014 07:25 PM, Ilya Verbin wrote: > Yes, initially the idea was to use it for look up the right function. > But now each DSO will call GOMP_offload_register, and pass unique > pointer to __OPENMP_TARGET__ (host_table) for this DSO. Then > gomp_register_images_for_device registers all this host tables in the > plugin. And when libgomp calls device_get_table_func, the plugin > returns the joint table for all DSO's. Why make a joint table? It seems better to use the __OPENMP_TARGET__ symbol to restrict lookups to the subset of symbols that could actually be found. BTW, I still expect that the lookup by ordering will turn out to be fundamentally unreliable and we'll need to use the unique id patch I posted a while ago. In that case using __OPENMP_TARGET__ as a first order key for the lookups eliminates any problem with duplicate names across multiple libraries. Bernd
2014-04-03 21:28 GMT+04:00 Bernd Schmidt <bernds@codesourcery.com>: > On 04/03/2014 07:25 PM, Ilya Verbin wrote: >> >> Yes, initially the idea was to use it for look up the right function. >> But now each DSO will call GOMP_offload_register, and pass unique >> pointer to __OPENMP_TARGET__ (host_table) for this DSO. Then >> gomp_register_images_for_device registers all this host tables in the >> plugin. And when libgomp calls device_get_table_func, the plugin >> returns the joint table for all DSO's. > > > Why make a joint table? It seems better to use the __OPENMP_TARGET__ symbol > to restrict lookups to the subset of symbols that could actually be found. > BTW, I still expect that the lookup by ordering will turn out to be > fundamentally unreliable and we'll need to use the unique id patch I posted > a while ago. In that case using __OPENMP_TARGET__ as a first order key for > the lookups eliminates any problem with duplicate names across multiple > libraries. > > > Bernd > In current implementation each gomp_device_descr contains one dev_splay_tree. And all addresses are inserted into this splay tree. There is no need to restrict lookup, because the addresses from multiple DSO's can't overlap. -- Ilya
Hi! On Thu, 3 Apr 2014 18:13:08 +0200, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 04/02/2014 10:36 AM, Thomas Schwinge wrote: > >> I see regressions in the libgomp testsuite for configurations where > >> offloading is not enabled: > >> > >> spawn [...]/build/gcc/xgcc -B[...]/build/gcc/ [...]/source/libgomp/testsuite/libgomp.c/for-3.c -B[...]/build/x86_64-unknown-linux-gnu/./libgomp/ -B[...]/build/x86_64-unknown-linux-gnu/./libgomp/.libs -I[...]/build/x86_64-unknown-linux-gnu/./libgomp -I[...]/source/libgomp/testsuite/.. -fmessage-length=0 -fno-diagnostics-show-caret -fdiagnostics-color=never -fopenmp -std=gnu99 -fopenmp -L[...]/build/x86_64-unknown-linux-gnu/./libgomp/.libs -lm -o ./for-3.exe > >> /tmp/ccGnT0ei.o: In function `main': > >> for-3.c:(.text+0x21032): undefined reference to `__OPENMP_TARGET__' > >> collect2: error: ld returned 1 exit status > >> > >> I suppose that's because [...] > > > > Workaround committed in r209015: > > > libgcc/ > > * crtstuff.c [!ENABLE_OFFLOADING] (__OPENMP_TARGET__): Define to > > NULL. > > The patch below should be a better fix, making the references to > __OPENMP_TARGET__ weak. Does this work for you? Yes, it does, thanks! Please revert my patch when committing yours. Oh, and please use ChangeLog.gomp files on gomp-4_0-branch; also please move the entries for your recent commits from the ChangeLog file(s) to the respective ChangeLog.gomp one(s). Grüße, Thomas
On 04/04/2014 07:55 AM, Thomas Schwinge wrote: > Hi! > > On Thu, 3 Apr 2014 18:13:08 +0200, Bernd Schmidt <bernds@codesourcery.com> wrote: >> The patch below should be a better fix, making the references to >> __OPENMP_TARGET__ weak. Does this work for you? > > Yes, it does, thanks! Please revert my patch when committing yours. > > > Oh, and please use ChangeLog.gomp files on gomp-4_0-branch; also please > move the entries for your recent commits from the ChangeLog file(s) to > the respective ChangeLog.gomp one(s). All done. Bernd
Index: gcc/omp-low.c =================================================================== --- gcc/omp-low.c (revision 429741) +++ gcc/omp-low.c (working copy) @@ -221,6 +221,28 @@ static tree scan_omp_1_op (tree *, int * *handled_ops_p = false; \ break; +static GTY(()) tree offload_symbol_decl; + +/* Get the __OPENMP_TARGET__ symbol. */ +static tree +get_offload_symbol_decl (void) +{ + if (!offload_symbol_decl) + { + tree decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, + get_identifier ("__OPENMP_TARGET__"), + ptr_type_node); + TREE_PUBLIC (decl) = 1; + DECL_EXTERNAL (decl) = 1; + DECL_WEAK (decl) = 1; + DECL_ATTRIBUTES (decl) + = tree_cons (get_identifier ("weak"), + NULL_TREE, DECL_ATTRIBUTES (decl)); + offload_symbol_decl = decl; + } + return offload_symbol_decl; +} + /* Convenience function for calling scan_omp_1_op on tree operands. */ static inline tree @@ -5148,11 +5170,7 @@ expand_oacc_offload (struct omp_region * } gimple g; - tree openmp_target - = build_decl (UNKNOWN_LOCATION, VAR_DECL, - get_identifier ("__OPENMP_TARGET__"), ptr_type_node); - TREE_PUBLIC (openmp_target) = 1; - DECL_EXTERNAL (openmp_target) = 1; + tree openmp_target = get_offload_symbol_decl (); tree fnaddr = build_fold_addr_expr (child_fn); g = gimple_build_call (builtin_decl_explicit (start_ix), 10, device, fnaddr, build_fold_addr_expr (openmp_target), @@ -8686,11 +8704,7 @@ expand_omp_target (struct omp_region *re } gimple g; - tree openmp_target - = build_decl (UNKNOWN_LOCATION, VAR_DECL, - get_identifier ("__OPENMP_TARGET__"), ptr_type_node); - TREE_PUBLIC (openmp_target) = 1; - DECL_EXTERNAL (openmp_target) = 1; + tree openmp_target = get_offload_symbol_decl (); if (kind == GF_OMP_TARGET_KIND_REGION) { tree fnaddr = build_fold_addr_expr (child_fn);