[hsa,1/10] Configury changes and new options

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 <mjambor@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  <mjambor@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(-)

On Thu, Dec 10, 2015 at 06:52:07PM +0100, Martin Jambor wrote:
> 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.

LGTM (and thanks for Richard for reviewing that).

> 2015-12-09  Martin Jambor  <mjambor@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

	Jakub