[PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Jakub Jelinek]

Hi!

As mentioned in the PR, %{ftree-parallelize-loops=*} expands to
all -ftree-parallelize-loops= options, not just the last one.
So greater_than_spec_func is actually called say for
-ftree-parallelize-loops=0 -ftree-parallelize-loops=2 with
- ftree-parallelize-loops=0 - ftree-parallelize-loops=2 1
(each whitespace separated sequence separate arg), but it asserts
it sees just 3 arguments.
Passing the - and ftree-parallelize-loops= stuff looks weird,
and we have %* that substitutes just the variable part of the option,
so in addition to fixing the case of multiple options I've also changed
%:gt() behaviour, so that it now gets just the numbers and compares the
last two of them.  So for the above options it would be called with
0 2 1
and would compare
2 > 1
and return "", or for
-ftree-parallelize-loops=2 -ftree-parallelize-loops=0 -ftree-parallelize-loops=1
would be
2 0 1 1
and compare
1 > 1
and return NULL. %:gt() is not used anywhere else, and has been introduced
only in GCC 6.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-16  Jakub Jelinek  <jakub@redhat.com>

	PR driver/69805
	* gcc.c (LINK_COMMAND_SPEC, GOMP_SELF_SPECS): Use
	:%* in %:gt() argument.
	(greater_than_spec_func): Adjust for expecting only numbers,
	if there are more than two numbers, compare the last two.

	* testsuite/libgomp.c/pr69805.c: New test.

--- gcc/gcc.c.jj	2016-02-15 22:22:51.000000000 +0100
+++ gcc/gcc.c	2016-02-16 09:35:03.579849080 +0100
@@ -1019,7 +1019,7 @@ proper position among the other output f
     %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}} \
     %{static:} %{L*} %(mfwrap) %(link_libgcc) " \
     VTABLE_VERIFICATION_SPEC " " SANITIZER_EARLY_SPEC " %o " CHKP_SPEC " \
-    %{fopenacc|fopenmp|%:gt(%{ftree-parallelize-loops=*} 1):\
+    %{fopenacc|fopenmp|%:gt(%{ftree-parallelize-loops=*:%*} 1):\
 	%:include(libgomp.spec)%(link_gomp)}\
     %{fcilkplus:%:include(libcilkrts.spec)%(link_cilkrts)}\
     %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
@@ -1183,7 +1183,7 @@ static const char *const multilib_defaul
    for targets that use different start files and suchlike.  */
 #ifndef GOMP_SELF_SPECS
 #define GOMP_SELF_SPECS \
-  "%{fopenacc|fopenmp|%:gt(%{ftree-parallelize-loops=*} 1): " \
+  "%{fopenacc|fopenmp|%:gt(%{ftree-parallelize-loops=*:%*} 1): " \
   "-pthread}"
 #endif
 
@@ -9764,7 +9764,7 @@ replace_extension_spec_func (int argc, c
   return result;
 }
 
-/* Returns "" if the n in ARGV[1] == -opt=<n> is greater than ARGV[2].
+/* Returns "" if ARGV[ARGC - 2] is greater than ARGV[ARGC-1].
    Otherwise, return NULL.  */
 
 static const char *
@@ -9775,29 +9775,13 @@ greater_than_spec_func (int argc, const
   if (argc == 1)
     return NULL;
 
-  gcc_assert (argc == 3);
-  gcc_assert (argv[0][0] == '-');
-  gcc_assert (argv[0][1] == '\0');
-
-  /* Point p to <n> in in -opt=<n>.  */
-  const char *p = argv[1];
-  while (true)
-    {
-      char c = *p;
-      if (c == '\0')
-	gcc_unreachable ();
+  gcc_assert (argc >= 2);
 
-      ++p;
-
-      if (c == '=')
-	break;
-    }
-
-  long arg = strtol (p, &converted, 10);
-  gcc_assert (converted != p);
+  long arg = strtol (argv[argc - 2], &converted, 10);
+  gcc_assert (converted != argv[argc - 2]);
 
-  long lim = strtol (argv[2], &converted, 10);
-  gcc_assert (converted != argv[2]);
+  long lim = strtol (argv[argc - 1], &converted, 10);
+  gcc_assert (converted != argv[argc - 1]);
 
   if (arg > lim)
     return "";
--- libgomp/testsuite/libgomp.c/pr69805.c.jj	2016-02-16 09:54:46.928527601 +0100
+++ libgomp/testsuite/libgomp.c/pr69805.c	2016-02-16 09:55:29.453941023 +0100
@@ -0,0 +1,9 @@
+/* PR driver/69805 */
+/* { dg-do link } */
+/* { dg-options "-ftree-parallelize-loops=1 -O2 -ftree-parallelize-loops=2" } */
+
+int
+main ()
+{
+  return 0;
+}

	Jakub

Re: [PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

On 16/02/16 16:24, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, %{ftree-parallelize-loops=*} expands to
> all -ftree-parallelize-loops= options, not just the last one.
> So greater_than_spec_func is actually called say for
> -ftree-parallelize-loops=0 -ftree-parallelize-loops=2 with
> - ftree-parallelize-loops=0 - ftree-parallelize-loops=2 1
> (each whitespace separated sequence separate arg), but it asserts
> it sees just 3 arguments.
> Passing the - and ftree-parallelize-loops= stuff looks weird,
> and we have %* that substitutes just the variable part of the option,
> so in addition to fixing the case of multiple options I've also changed
> %:gt() behaviour, so that it now gets just the numbers and compares the
> last two of them.  So for the above options it would be called with
> 0 2 1
> and would compare
> 2 > 1
> and return "", or for
> -ftree-parallelize-loops=2 -ftree-parallelize-loops=0 -ftree-parallelize-loops=1
> would be
> 2 0 1 1
> and compare
> 1 > 1
> and return NULL. %:gt() is not used anywhere else, and has been introduced
> only in GCC 6.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-02-16  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR driver/69805
> 	* gcc.c (LINK_COMMAND_SPEC, GOMP_SELF_SPECS): Use
> 	:%* in %:gt() argument.
> 	(greater_than_spec_func): Adjust for expecting only numbers,
> 	if there are more than two numbers, compare the last two.
>
> 	* testsuite/libgomp.c/pr69805.c: New test.
>

Here's the documentation entry for the gt spec function (I forgot to add 
it when introducing the function), using the new semantics.

Copy-pasting from the resulting .info viewed in emacs for a 
human-readable version:
...
      'gt'
           The 'gt' (greater than) function takes one or more arguments.
           It returns either NULL or the empty string.  If it has one
           argument, it returns NULL.  If it has two arguments, it
           compares them: it returns the empty string if the first
           argument is greater than the second argument, otherwise it
           returns NULL.  If it has more than two arguments, it behaves
           as if only the last two arguments were passed.  It can be used
           f.i. as 'S' in a spec directive %{'S':'X'}: if 'S' is NULL,
           the empty string is substituted, and if 'S' is the empty
           string, 'X' is substituted.

                %:gt(%{fsome-option-value=*:%*} 1)
...

OK for stage4 trunk?

Thanks,
- Tom


[-- Attachment #2: 0001-Add-documentation-for-spec-function-gt-in-invoke.texi.patch --]
[-- Type: text/x-patch, Size: 1309 bytes --]

Add documentation for spec function gt in invoke.texi

2016-02-16  Tom de Vries  <tom@codesourcery.com>

	* doc/invoke.texi (@node Spec Files): Document spec function gt.

---
 gcc/doc/invoke.texi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 666d976..2da7a72 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -24783,6 +24783,20 @@ Use "-Wa,OPTION" to pass "OPTION" to the assembler.
 
 It is used to separate compiler options from assembler options
 in the @option{--target-help} output.
+
+@item @code{gt}
+The @code{gt} (greater than) function takes one or more arguments.  It returns
+either NULL or the empty string.  If it has one argument, it returns NULL.@  If
+it has two arguments, it compares them: it returns the empty string if the first
+argument is greater than the second argument, otherwise it returns NULL.@  If it
+has more than two arguments, it behaves as if only the last two arguments were
+passed.  It can be used f.i.@ as @code{S} in a spec directive
+%@{@code{S}:@code{X}@}: if @code{S} is NULL, the empty string is substituted,
+and if @code{S} is the empty string, @code{X} is substituted.
+
+@smallexample
+%:gt(%@{fsome-option-value=*:%*@} 1)
+@end smallexample
 @end table
 
 @item %@{@code{S}@}

Re: [PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Sandra Loosemore]

On 02/17/2016 12:14 AM, Tom de Vries wrote:
>
> Here's the documentation entry for the gt spec function (I forgot to add
> it when introducing the function), using the new semantics.
>
> Copy-pasting from the resulting .info viewed in emacs for a
> human-readable version:
> ...
>       'gt'
>            The 'gt' (greater than) function takes one or more arguments.
>            It returns either NULL or the empty string.  If it has one
>            argument, it returns NULL.  If it has two arguments, it
>            compares them: it returns the empty string if the first
>            argument is greater than the second argument, otherwise it
>            returns NULL.  If it has more than two arguments, it behaves
>            as if only the last two arguments were passed.  It can be used
>            f.i. as 'S' in a spec directive %{'S':'X'}: if 'S' is NULL,
>            the empty string is substituted, and if 'S' is the empty
>            string, 'X' is substituted.
>
>                 %:gt(%{fsome-option-value=*:%*} 1)
> ...
>
> OK for stage4 trunk?

I'm not an expert on spec strings....  but from a user perspective, what 
is the difference between "NULL" and "the empty string"?  The other spec 
escapes are documented in terms of pattern substitutions at the point 
where the escape appears in the spec string.

-Sandra

Re: [PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Jakub Jelinek]

On Wed, Feb 17, 2016 at 09:07:42AM -0700, Sandra Loosemore wrote:
> On 02/17/2016 12:14 AM, Tom de Vries wrote:
> >
> >Here's the documentation entry for the gt spec function (I forgot to add
> >it when introducing the function), using the new semantics.
> >
> >Copy-pasting from the resulting .info viewed in emacs for a
> >human-readable version:
> >...
> >      'gt'
> >           The 'gt' (greater than) function takes one or more arguments.
> >           It returns either NULL or the empty string.  If it has one
> >           argument, it returns NULL.  If it has two arguments, it
> >           compares them: it returns the empty string if the first
> >           argument is greater than the second argument, otherwise it
> >           returns NULL.  If it has more than two arguments, it behaves
> >           as if only the last two arguments were passed.  It can be used
> >           f.i. as 'S' in a spec directive %{'S':'X'}: if 'S' is NULL,
> >           the empty string is substituted, and if 'S' is the empty
> >           string, 'X' is substituted.
> >
> >                %:gt(%{fsome-option-value=*:%*} 1)
> >...
> >
> >OK for stage4 trunk?
> 
> I'm not an expert on spec strings....  but from a user perspective, what is
> the difference between "NULL" and "the empty string"?  The other spec
> escapes are documented in terms of pattern substitutions at the point where
> the escape appears in the spec string.

NULL vs. "" is the internal (C level representation) of %:fn() function
returned false vs. true for the purpose of e.g. %{%:fn():...}.

	Jakub

Re: [PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Tom de Vries]

On 17/02/16 17:07, Sandra Loosemore wrote:
> On 02/17/2016 12:14 AM, Tom de Vries wrote:
>>
>> Here's the documentation entry for the gt spec function (I forgot to add
>> it when introducing the function), using the new semantics.
>>
>> Copy-pasting from the resulting .info viewed in emacs for a
>> human-readable version:
>> ...
>>       'gt'
>>            The 'gt' (greater than) function takes one or more arguments.
>>            It returns either NULL or the empty string.  If it has one
>>            argument, it returns NULL.  If it has two arguments, it
>>            compares them: it returns the empty string if the first
>>            argument is greater than the second argument, otherwise it
>>            returns NULL.  If it has more than two arguments, it behaves
>>            as if only the last two arguments were passed.  It can be used
>>            f.i. as 'S' in a spec directive %{'S':'X'}: if 'S' is NULL,
>>            the empty string is substituted, and if 'S' is the empty
>>            string, 'X' is substituted.
>>
>>                 %:gt(%{fsome-option-value=*:%*} 1)
>> ...
>>
>> OK for stage4 trunk?
>
> I'm not an expert on spec strings....  but from a user perspective, what
> is the difference between "NULL" and "the empty string"?  The other spec
> escapes are documented in terms of pattern substitutions at the point
> where the escape appears in the spec string.

Hi Sandra,

yes, that's a good point.


I.

The first time we used this type of "NULL"/"empty string" distinction 
was for the sanitize spec function, introduced here ( 
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=0016189f560b1dc86734d36826be891562da1c46;hp=e9d78f5ab2b5a1cc50cd20ca2da199f83258b153 
) in the ubsan branch, which was later merged to trunk at r202113 (patch 
submitted here: https://gcc.gnu.org/ml/gcc-patches/2013-07/msg00510.html ).

The commit adds this text to 'The Specs Language' bit in gcc.c:
...
  %{%:function(args):X}
          Call function named FUNCTION with args ARGS.  If the function
          returns non-NULL, then X is substituted, if it returns
          NULL, it isn't substituted.
...

I suppose we should document this behavior first in invoke.texi, and we 
could choose user-oriented names for the NULL/non-NULL distinction.


II.

Furthermore, the implementation seems to be inconsistent with the 
specification. If I modify greater_than_spec_func to return non-NULL, 
but not the empty string:
...
@@ -9800,7 +9803,7 @@ greater_than_spec_func
    gcc_assert (converted != argv[2]);

    if (arg > lim)
-    return "";
+    return "true";

    return NULL;
  }
...

I get:
...
$ gcc hello.c -O2 -ftree-parallelize-loops=2
gcc: fatal error: switch ‘true’ does not start with ‘-’
compilation terminated.
...

This (otherwise untested) patch fixes that:
...
@@ -6068,10 +6068,13 @@ handle_spec_function
    /* p now points to just past the end of the spec function
       expression.  */

    funcval = eval_spec_function (func, args);
-  if (funcval != NULL && do_spec_1 (funcval, 0, NULL) < 0)
-    p = NULL;
    if (retval_nonnull)
      *retval_nonnull = funcval != NULL;
+  else
+    {
+      if (funcval != NULL && do_spec_1 (funcval, 0, NULL) < 0)
+       p = NULL;
+    }

    free (func);
    free (args);
...


III.

So, a spec function can return:
a: NULL,
b: empty string, and
c: non-empty string.

In the context of evaluating a spec function as a condition, NULL 
designates false, empty string designates true and if we fix the problem 
mentioned at II then also non-empty string designates true, so:
- false (a)
- true (b, c)

In the context of evaluating a spec function for the resulting string, 
NULL means an empty string (AFAICT), just like an empty string, so we have:
- empty (a, b)
- non-empty (c)

I think that's going to be hard to explain to the user.

It seems originally ( 
https://gcc.gnu.org/ml/gcc-patches/2013-06/msg01100.html ) the 
empty/non-empty string distinction was proposed for the semantics of 
'%{%:function(args):X}'. Perhaps we should consider a rewrite using that 
distinction.

Thanks,
- Tom

Re: [PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Tom de Vries]

On 16/02/16 16:24, Jakub Jelinek wrote:
> Passing the - and ftree-parallelize-loops= stuff looks weird,
> and we have %* that substitutes just the variable part of the option,
> so in addition to fixing the case of multiple options I've also changed
> %:gt() behaviour, so that it now gets just the numbers and compares the
> last two of them.

FTR, version-compare uses yet another variant: It does not expect to be 
called with an %{S*} or %{S*:%*} expansion, but passes S as an argument:
...
%:version-compare(>= 10.3 mmacosx-version-min= -lmx)
...
and the spec function itself takes care of finding the last value.

Thanks,
- Tom

Re: [PATCH] Fix driver handling of multiple -ftree-parallelize-loops=<n> options (PR driver/69805)

[Jeff Law]

On 02/16/2016 08:24 AM, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, %{ftree-parallelize-loops=*} expands to
> all -ftree-parallelize-loops= options, not just the last one.
> So greater_than_spec_func is actually called say for
> -ftree-parallelize-loops=0 -ftree-parallelize-loops=2 with
> - ftree-parallelize-loops=0 - ftree-parallelize-loops=2 1
> (each whitespace separated sequence separate arg), but it asserts
> it sees just 3 arguments.
> Passing the - and ftree-parallelize-loops= stuff looks weird,
> and we have %* that substitutes just the variable part of the option,
> so in addition to fixing the case of multiple options I've also changed
> %:gt() behaviour, so that it now gets just the numbers and compares the
> last two of them.  So for the above options it would be called with
> 0 2 1
> and would compare
> 2 > 1
> and return "", or for
> -ftree-parallelize-loops=2 -ftree-parallelize-loops=0 -ftree-parallelize-loops=1
> would be
> 2 0 1 1
> and compare
> 1 > 1
> and return NULL. %:gt() is not used anywhere else, and has been introduced
> only in GCC 6.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-02-16  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR driver/69805
> 	* gcc.c (LINK_COMMAND_SPEC, GOMP_SELF_SPECS): Use
> 	:%* in %:gt() argument.
> 	(greater_than_spec_func): Adjust for expecting only numbers,
> 	if there are more than two numbers, compare the last two.
>
> 	* testsuite/libgomp.c/pr69805.c: New test.
I'll note that Tom ack'd in c#3, and he certainly know this 
functionality better than I.  So OK for trunk.

Jeff