Re: [PATCH] LTO: fallback to -flto=N if -flto=jobserver does not work.

["Martin Liška" <mliska@suse.cz>]

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

On 4/22/21 2:47 PM, Richard Biener wrote:
> On Thu, Apr 22, 2021 at 2:21 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 4/22/21 1:19 PM, Richard Biener wrote:
>>> On Thu, Apr 22, 2021 at 11:02 AM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> On 4/22/21 10:04 AM, Richard Biener wrote:
>>>>> On Wed, Apr 21, 2021 at 3:08 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>>
>>>>>> When -flto=jobserver is used and we cannot detect job server, then we can
>>>>>> still fallbackto -flto=N mode.
>>>>>>
>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>
>>>>>> Ready to be installed?
>>>>>
>>>>> I think this behavior needs to be documented - it falls back to a less
>>>>> conservative (possibly system overloading) mode - which IMHO is
>>>>> non-obvious and IMHO we shouldn't do.
>>>>
>>>> Sure, I'm sending corresponding patch. Note that it's quite common mistake
>>>> that '+' is missing in Makefile rule. That was motivation for my change.
>>>
>>> Sure, but that change won't get this fixed.
>>
>> It will as linker command line will contain (-flto=jobserver) and LTO will fallback to -flto=N.
>>
>>> IMHO we should eventually
>>> emit diagnostic like
>>>
>>> warning: could not find jobserver, compiling N jobs serially
>>>
>>> once N > 1 (or 2?).
>>
>> We do that now (for all N):
>> lto-wrapper: warning: jobserver is not available: ‘MAKEFLAGS’ environment variable is unset
>>
>>
>>> Likewise if people just use -flto and auto-detection
>>> finds nothing:
>>
>> -flto != -flto=auto
>>
>> Yes, -flto is a serial linking and we can emit a warning.
> 
> I'd avoid warning if there's just a single ltrans unit.

That's doable and I've just done that in the attached patch.
Two disadvantages:
- one needs waiting for the warning after WPA
- source change (>1 LTRANS) can trigger the warning

> 
>>> warning: using serial compilation of N LTRANS jobs
>>> note: refer to http://.... for how to use parallel compile
>>>
>>> using the URL diagnostics to point to -flto=... documentation.
>>
>> What about making that a proper warning (-Wlto)? We have diagnostics infrastructure
>> that prints URL links.
> 
> Note that drivers like lto-wrapper do not have fully initialized diagnostic
> machinery and use a "different" set of overloads (likewise gen* programs).

I managed printing the warning:

lto-wrapper: warning: jobserver is not available: ‘MAKEFLAGS’ environment variable is unset

lto-wrapper: note: see the ‘-flto’ option documentation for more information


and

lto-wrapper: warning: using serial compilation of 128 LTRANS jobs

lto-wrapper: note: see the ‘-flto’ option documentation for more information


> 
>>>
>>> That is, teach users rather than second-guessing and eventually
>>> blowing things up.  IMHO only the jobserver mode is safe to
>>> automatically use.
>>
>> Well, -flto=auto is also fine and document. I think there is no possibility
>> auto CPU deduction can fail. So -flto=jobserver (with missing make job server)
>> and -flto (equal to -flto=1) worth emitting a warning.
>>
>> What do you think?
> 
> Yes, that sounds reasonable.  I suspect that people might want to see
> -flto default to -flto=auto but then I don't think that's good because there's
> no system wide job scheduler limiting things (systemd-jobserver anyone?)

Done that.

Thoughts?
Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>>         * lto-wrapper.c (run_gcc): When -flto=jobserver is used, but the
>>>>>>         makeserver cannot be detected, then use -flto=N fallback.
>>>>>> ---
>>>>>>  gcc/lto-wrapper.c | 3 ++-
>>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
>>>>>> index 03a5922f8ea..0b626d7c811 100644
>>>>>> --- a/gcc/lto-wrapper.c
>>>>>> +++ b/gcc/lto-wrapper.c
>>>>>> @@ -1585,8 +1585,9 @@ run_gcc (unsigned argc, char *argv[])
>>>>>>        if (jobserver && jobserver_error != NULL)
>>>>>>         {
>>>>>>           warning (0, jobserver_error);
>>>>>> -         parallel = 0;
>>>>>> +         /* Fall back to auto parallelism.  */
>>>>>>           jobserver = 0;
>>>>>> +         auto_parallel = 1;
>>>>>>         }
>>>>>>        else if (!jobserver && jobserver_error == NULL)
>>>>>>         {
>>>>>> --
>>>>>> 2.31.1
>>>>>>
>>>>
>>


[-- Attachment #2: 0001-Print-warning-diagnostics-for-flto-issues.patch --]
[-- Type: text/x-patch, Size: 4042 bytes --]

From 7cd89e17c49c027027e2aed1722c0758331ac7ac Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 22 Apr 2021 16:27:19 +0200
Subject: [PATCH] Print warning diagnostics for -flto issues.

gcc/ChangeLog:

	* lto-wrapper.c (print_lto_docs_link): New function.
	(run_gcc): Print warning about missing job server detection
	after we know NR of partitions. Do the same for -flto{,=1}.
	* opts.c (get_option_html_page): Support -flto option.
---
 gcc/lto-wrapper.c | 39 +++++++++++++++++++++++++++++++++++++--
 gcc/opts.c        |  6 +++++-
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 0b626d7c811..f7ca084ef6c 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -48,6 +48,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "simple-object.h"
 #include "lto-section-names.h"
 #include "collect-utils.h"
+#include "opts-diagnostic.h"
 
 /* Environment variable, used for passing the names of offload targets from GCC
    driver to lto-wrapper.  */
@@ -1335,6 +1336,23 @@ jobserver_active_p (void)
     return JS_PREFIX "cannot access %<" JS_NEEDLE "%> file descriptors";
 }
 
+/* Print link to -flto documentation with a hint message.  */
+
+void
+print_lto_docs_link ()
+{
+  const char *url = get_option_url (NULL, OPT_flto);
+
+  pretty_printer pp;
+  pp.url_format = URL_FORMAT_DEFAULT;
+  pp_string (&pp, "see the ");
+  pp_begin_url (&pp, url);
+  pp_string (&pp, "%<-flto%> option documentation");
+  pp_end_url (&pp);
+  pp_string (&pp, " for more information");
+  inform (UNKNOWN_LOCATION, pp_formatted_text (&pp));
+}
+
 /* Test that a make command is present and working, return true if so.  */
 
 static bool
@@ -1369,8 +1387,10 @@ run_gcc (unsigned argc, char *argv[])
   char *collect_gcc_options;
   int parallel = 0;
   int jobserver = 0;
+  bool jobserver_requested = false;
   int auto_parallel = 0;
   bool no_partition = false;
+  const char *jobserver_error = NULL;
   struct cl_decoded_option *fdecoded_options = NULL;
   struct cl_decoded_option *offload_fdecoded_options = NULL;
   unsigned int fdecoded_options_count = 0;
@@ -1522,6 +1542,7 @@ run_gcc (unsigned argc, char *argv[])
 	    {
 	      parallel = 1;
 	      jobserver = 1;
+	      jobserver_requested = true;
 	    }
 	  else if (strcmp (option->arg, "auto") == 0)
 	    {
@@ -1576,15 +1597,15 @@ run_gcc (unsigned argc, char *argv[])
     {
       lto_mode = LTO_MODE_LTO;
       jobserver = 0;
+      jobserver_requested = false;
       auto_parallel = 0;
       parallel = 0;
     }
   else
     {
-      const char *jobserver_error = jobserver_active_p ();
+      jobserver_error = jobserver_active_p ();
       if (jobserver && jobserver_error != NULL)
 	{
-	  warning (0, jobserver_error);
 	  /* Fall back to auto parallelism.  */
 	  jobserver = 0;
 	  auto_parallel = 1;
@@ -1904,6 +1925,20 @@ cont:
       maybe_unlink (ltrans_output_file);
       ltrans_output_file = NULL;
 
+      if (nr > 1)
+	{
+	  if (jobserver_requested && jobserver_error != NULL)
+	    {
+	      warning (0, jobserver_error);
+	      print_lto_docs_link ();
+	    }
+	  else if (parallel == 0)
+	    {
+	      warning (0, "using serial compilation of %d LTRANS jobs", nr);
+	      print_lto_docs_link ();
+	    }
+	}
+
       if (parallel)
 	{
 	  makefile = make_temp_file (".mk");
diff --git a/gcc/opts.c b/gcc/opts.c
index 24bb64198c8..fe6fddbf095 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -3243,9 +3243,13 @@ get_option_html_page (int option_index)
   const cl_option *cl_opt = &cl_options[option_index];
 
   /* Analyzer options are on their own page.  */
-  if (strstr(cl_opt->opt_text, "analyzer-"))
+  if (strstr (cl_opt->opt_text, "analyzer-"))
     return "gcc/Static-Analyzer-Options.html";
 
+  /* Handle -flto= option.  */
+  if (strstr (cl_opt->opt_text, "flto"))
+    return "gcc/Optimize-Options.html";
+
 #ifdef CL_Fortran
   if ((cl_opt->flags & CL_Fortran) != 0
       /* If it is option common to both C/C++ and Fortran, it is documented
-- 
2.31.1