Re: [PATCH 6/n] OpenMP 4.0 offloading infrastructure: option handling

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

On Sat, 11 Oct 2014 18:49:00 +0400, Ilya Verbin <iverbin@gmail.com> wrote:
> This is the last common infrastructure patch in the series.

> 1. -foffload=<targets>=<options>
>    By default, GCC will build offload images for all offload targets specified
> in configure, with non-target-specific options passed to host compiler.
> This option is used to control offload targets and options for them.
> 
> It can be used in a few ways:
> * -foffload=disable
>   Tells GCC to disable offload support.
>   OpenMP target regions will be run in host fallback mode.
> * -foffload=<targets>
>   Tells GCC to build offload images for <targets>.
>   They will be built with non-target-specific options passed to host compiler.
> * -foffload=<options>
>   Tells GCC to build offload images for all targets specified in configure. 
>   They will be built with non-target-specific options passed to host compiler
>   plus <options>.
> * -foffload=<targets>=<options>
>   Tells GCC to build offload images for <targets>.
>   They will be built with non-target-specific options passed to host compiler
>   plus <options>.
> 
> Options specified by -foffload are appended to the end of option set, so in case
> of option conflicts they have more priority.

(<https://gcc.gnu.org/PR67300>, "--foffload* undocumented", has recently
been filed.)

(In the following, "intelmic" is short for
"x86_64-intelmicemul-linux-gnu", and "nvptx" is short for "nvptx-none".)

What is the syntax to use for building both intelmic and nvptx offloading
code?  I understand we allow for separate -foffload=intelmic
-foffload=nvptx options.  Do we also intend to allow
-foffload=intelmic,nvptx or -foffload=intelmic:nvptx?

And then, we allow for specifying offloading compiler options with
-foffload=intelmic=[...] and -foffload=nvptx=[...]; do we also intend to
allow -foffload=intelmic,nvptx=[...] (do the options apply to nvptx only,
or to both intelmic and nvptx?), and/or
-foffload=intelmic=[...],nvptx=[...], and/or
-foffload=intelmic:nvptx=[...] (which "looks a bit like" the options
ought to apply to nvptx only -- or to both intelmic and nvptx?), and/or
-foffload=intelmic=[...]:nvptx=[...]?

Given the code in gcc/configure.ac:

       939  for tgt in `echo $enable_offload_targets | sed 's/,/ /g'`; do
       940    tgt=`echo $tgt | sed 's/=.*//'`
       941    if test x"$offload_targets" = x; then
       942      offload_targets=$tgt
       943    else
       944      offload_targets="$offload_targets:$tgt"
       945    fi
       946  done
       947  AC_DEFINE_UNQUOTED(OFFLOAD_TARGETS, "$offload_targets",
       948    [Define to hold the list of target names suitable for offloading.])

..., we constuct OFFLOAD_TARGETS with the offload targets delimited by
colons, but when parsing -foffload options:

> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c

> +/* Parse -foffload option argument.  */
> +
> +static void
> +handle_foffload_option (const char *arg)
> +{
> +  [...]

..., we look for comma delimiters in the configure-time OFFLOAD_TARGETS:

      3612        /* Check that GCC is configured to support the offload target.  */
      3613        c = OFFLOAD_TARGETS;
      3614        while (c)
      3615          {
      3616            n = strchr (c, ',');
      3617            if (n == NULL)
      3618              n = strchr (c, '\0');
      3619  
      3620            if (next - cur == n - c && strncmp (target, c, n - c) == 0)
      3621              break;
      3622  
      3623            c = *n ? n + 1 : NULL;
      3624          }
      3625  
      3626        if (!c)
      3627          fatal_error (input_location,
      3628                       "GCC is not configured to support %s as offload target",
      3629                       target);

So, this code will not do the right thing when configured with
--enable-offload-targets=intelmic,nvptx (thus,
OFFLOAD_TARGETS=intelmic:nvptx): using -foffload=nvptx will then result
in "xgcc: fatal error: GCC is not configured to support nvptx as offload
target".

If I'm understanding the following code correctly, this supports the idea
that the intention has been for -foffload=[targets]=[options] to separate
the targets by commas, and separate the options by spaces -- is that
correct?

> --- a/gcc/lto-wrapper.c
> +++ b/gcc/lto-wrapper.c

> +/* Extract options for TARGET offload compiler from OPTIONS and append
> +   them to ARGV_OBSTACK.  */
> +
> +static void
> +append_offload_options (obstack *argv_obstack, const char *target,
> +			struct cl_decoded_option *options,
> +			unsigned int options_count)
> +{
> +  [...]

       571  /* Extract options for TARGET offload compiler from OPTIONS and append
       572     them to ARGV_OBSTACK.  */
       573  
       574  static void
       575  append_offload_options (obstack *argv_obstack, const char *target,
       576                          struct cl_decoded_option *options,
       577                          unsigned int options_count)
       578  {
       579    for (unsigned i = 0; i < options_count; i++)
       580      {
       581        const char *cur, *next, *opts;
       582        char **argv;
       583        unsigned argc;
       584        struct cl_decoded_option *option = &options[i];
       585  
       586        if (option->opt_index != OPT_foffload_)
       587          continue;
       588  
       589        /* If option argument starts with '-' then no target is specified.  That
       590           means offload options are specified for all targets, so we need to
       591           append them.  */
       592        if (option->arg[0] == '-')
       593          opts = option->arg;
       594        else
       595          {
       596            opts = strchr (option->arg, '=');
       597            if (!opts)
       598              continue;
       599  
       600            cur = option->arg;
       601  
       602            while (cur < opts)
       603              {
       604                next = strchr (cur, ',');
       605                if (next == NULL)
       606                  next = opts;
       607                next = (next > opts) ? opts : next;
       608  
       609                if (strlen (target) == (size_t) (next - cur)
       610                    && strncmp (target, cur, next - cur) == 0)
       611                  break;
       612  
       613                cur = next + 1;
       614              }
       615  
       616            if (cur >= opts)
       617              continue;
       618  
       619            opts++;
       620          }
       621  
       622        argv = buildargv (opts);
       623        for (argc = 0; argv[argc]; argc++)
       624          obstack_ptr_grow (argv_obstack, argv[argc]);
       625      }
       626  }

In this case, the other use of OFFLOAD_TARGETS in gcc/gcc.c:

      7570  /* Set up to remember the names of offload targets.  */
      7571  
      7572  void
      7573  driver::maybe_putenv_OFFLOAD_TARGETS () const
      7574  {
      7575    const char *targets = offload_targets;
      7576  
      7577    /* If no targets specified by -foffload, use all available targets.  */
      7578    if (!targets)
      7579      targets = OFFLOAD_TARGETS;
      7580  
      7581    if (strlen (targets) > 0)
      7582      {
      7583        obstack_grow (&collect_obstack, "OFFLOAD_TARGET_NAMES=",
      7584                      sizeof ("OFFLOAD_TARGET_NAMES=") - 1);
      7585        obstack_grow (&collect_obstack, targets,
      7586                      strlen (targets) + 1);
      7587        xputenv (XOBFINISH (&collect_obstack, char *));
      7588      }
      7589  
      7590    free (offload_targets);
      7591  }

... also isn't correct, because the environment variable
OFFLOAD_TARGET_NAMES in contrast is meant to separate the targets with
colons instead of commas (parsing code in
gcc/lto-wrapper.c:parse_env_var), so we can't just set the environment
variable OFFLOAD_TARGET_NAMES to OFFLOAD_TARGETS here.  The following
patch should address this (but I have not yet tested it):

diff --git gcc/gcc.c gcc/gcc.c
index 757bfc9..ad210c5a5 100644
--- gcc/gcc.c
+++ gcc/gcc.c
@@ -284,7 +284,8 @@ static const char *const spec_version = DEFAULT_TARGET_VERSION;
 static const char *spec_machine = DEFAULT_TARGET_MACHINE;
 static const char *spec_host_machine = DEFAULT_REAL_TARGET_MACHINE;
 
-/* List of offload targets.  */
+/* List of offload targets.  Separated by colon.  Empty string for
+   -foffload=disable.  */
 
 static char *offload_targets = NULL;
 
@@ -4376,6 +4377,11 @@ process_command (unsigned int decoded_options_count,
 			   CL_DRIVER, &handlers, global_dc);
     }
 
+  /* If the user didn't specify any, default to all configured offload
+     targets.  */
+  if (offload_targets == NULL)
+    handle_foffload_option (OFFLOAD_TARGETS);
+
   if (output_file
       && strcmp (output_file, "-") != 0
       && strcmp (output_file, HOST_BIT_BUCKET) != 0)
@@ -7572,22 +7578,17 @@ driver::maybe_putenv_COLLECT_LTO_WRAPPER () const
 void
 driver::maybe_putenv_OFFLOAD_TARGETS () const
 {
-  const char *targets = offload_targets;
-
-  /* If no targets specified by -foffload, use all available targets.  */
-  if (!targets)
-    targets = OFFLOAD_TARGETS;
-
-  if (strlen (targets) > 0)
+  if (offload_targets && offload_targets[0] != '\0')
     {
       obstack_grow (&collect_obstack, "OFFLOAD_TARGET_NAMES=",
 		    sizeof ("OFFLOAD_TARGET_NAMES=") - 1);
-      obstack_grow (&collect_obstack, targets,
-		    strlen (targets) + 1);
+      obstack_grow (&collect_obstack, offload_targets,
+		    strlen (offload_targets) + 1);
       xputenv (XOBFINISH (&collect_obstack, char *));
     }
 
   free (offload_targets);
+  offload_targets = NULL;
 }
 
 /* Reject switches that no pass was interested in.  */


GrÃÃe,
 Thomas

Attachment: signature.asc
Description: PGP signature