Hi, On Tue, Dec 08, 2015 at 10:43:15PM +0000, Richard Sandiford wrote: > [Sorry for the low-quality review, was just reading out of interest...] > > Martin Jambor <mjam...@suse.cz> writes: > > +If you configure GCC with HSA offloading but do not have the HSA > > +run-time library installed in a standard location then you can > > +explicitely specify the directory where they are installed. The > > typo: explicitly
oops. For some reason, my spell-checker accepts this typo. I will fix it. > > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c > > index e4772d1..5609207 100644 > > --- a/gcc/lto-wrapper.c > > +++ b/gcc/lto-wrapper.c > > @@ -745,6 +745,11 @@ compile_images_for_offload_targets (unsigned in_argc, > > char *in_argv[], > > offload_names = XCNEWVEC (char *, num_targets + 1); > > for (unsigned i = 0; i < num_targets; i++) > > { > > + /* HSA does not use LTO-like streaming and a different compiler, skip > > + it. */ > > + if (strncmp(names[i], "hsa", 3) == 0) > > + continue; > > + > > offload_names[i] > > = compile_offload_image (names[i], compiler_path, in_argc, in_argv, > > compiler_opts, compiler_opt_count, > > Looks like this would cause the caller loop: > > if (offload_names) > { > find_offloadbeginend (); > for (i = 0; offload_names[i]; i++) > printf ("%s\n", offload_names[i]); > free_array_of_ptrs ((void **) offload_names, i); > } > > to terminate early if there was another target after hsa. > Good catch. I have modified this code so that it never leaves any holes in offload_names[i]. > names[i] is null-terminated, so it looks like you're deliberately > allowing anything that starts with "hsa" here, but: Right, and that was probably a mistake, I have changed the check to simple strcmp. > > > diff --git a/gcc/opts.c b/gcc/opts.c > > index 874c84f..5647f0c 100644 > > --- a/gcc/opts.c > > +++ b/gcc/opts.c > > @@ -1906,8 +1906,35 @@ common_handle_option (struct gcc_options *opts, > > break; > > > > case OPT_foffload_: > > - /* Deferred. */ > > - break; > > + { > > + const char *p = arg; > > + opts->x_flag_disable_hsa = true; > > + while (*p != 0) > > + { > > + const char *comma = strchr (p, ','); > > + > > + if ((strncmp (p, "disable", 7) == 0) > > + && (p[7] == ',' || p[7] == '\0')) > > + { > > + opts->x_flag_disable_hsa = true; > > + break; > > + } > > + > > + if ((strncmp (p, "hsa", 3) == 0) > > + && (p[3] == ',' || p[3] == '\0')) > > + { > > +#ifdef ENABLE_HSA > > + opts->x_flag_disable_hsa = false; > > +#else > > + sorry ("HSA has not been enabled during configuration"); > > +#endif > > ...here you only allow "hsa" itself. > > (Not your fault, but: do we have any documentation for -foffload > and -foffload-abi? Couldn't see any in the texi files.) Yes, that is actually PR 67300. However, I do not understand the more complex forms the parameter can take enough to attempt to fix it. In order to address all for you concerns, I am going to install the following on the branch. Thanks for the feedback, Martin 2015-12-09 Martin Jambor <mjam...@suse.cz> * lto-wrapper.c (compile_images_for_offload_targets): Do not leave holes in offload_names. Use strcmp instead strncmp. * doc/install.texi (--with-hsa-runtime): Fix typo. --- gcc/doc/install.texi | 2 +- gcc/lto-wrapper.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi index afd891c..a85a063 100644 --- a/gcc/doc/install.texi +++ b/gcc/doc/install.texi @@ -1993,7 +1993,7 @@ compiler will emit the accelerator code, no path should be specified. If you configure GCC with HSA offloading but do not have the HSA run-time library installed in a standard location then you can -explicitely specify the directory where they are installed. The +explicitly specify the directory where they are installed. The @option{--with-hsa-runtime=@/@var{hsainstalldir}} option is a shorthand for @option{--with-hsa-runtime-lib=@/@var{hsainstalldir}/lib} and diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 5609207..5b58fd6 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -736,6 +736,7 @@ compile_images_for_offload_targets (unsigned in_argc, char *in_argv[], return; unsigned num_targets = parse_env_var (target_names, &names, NULL); + int next_name_entry = 0; const char *compiler_path = getenv ("COMPILER_PATH"); if (!compiler_path) goto out; @@ -747,16 +748,17 @@ compile_images_for_offload_targets (unsigned in_argc, char *in_argv[], { /* HSA does not use LTO-like streaming and a different compiler, skip it. */ - if (strncmp(names[i], "hsa", 3) == 0) + if (strcmp (names[i], "hsa") == 0) continue; - offload_names[i] + offload_names[next_name_entry] = compile_offload_image (names[i], compiler_path, in_argc, in_argv, compiler_opts, compiler_opt_count, linker_opts, linker_opt_count); - if (!offload_names[i]) + if (!offload_names[next_name_entry]) fatal_error (input_location, "problem with building target image for %s\n", names[i]); + next_name_entry++; } out: -- 2.6.3