Re: [hsa 1/10] Configury changes and new options

[Richard Sandiford <rdsandiford@googlemail.com>]

[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

> 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.

names[i] is null-terminated, so it looks like you're deliberately
allowing anything that starts with "hsa" here, but:

> 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.)

Thanks,
Richard