diff mbox series

[1/4] gensupport: Centralize machine-generator output management

Message ID 20260805143526.27275-1-ktkachov@nvidia.com
State New
Headers
Series [1/4] gensupport: Centralize machine-generator output management |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap success Build passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap success Build passed

Commit Message

Kyrylo Tkachov Aug. 5, 2026, 2:35 p.m. UTC 
  From: Kyrylo Tkachov <ktkachov@nvidia.com>

genemit and genrecog use the same size-based output selection, but each
generator owns its file names, opens files during option parsing, and closes
them separately.  Adding more partitioned generators would copy this logic.

Put the common output record and lifecycle helpers in gensupport.  The helpers
validate names, delay file opening until option parsing is complete, select
only partition outputs by current size, and close every output with a useful
diagnostic.  Fixed outputs, such as headers, use the same record but do not
participate in selection.

Convert genemit and genrecog to the common interface.  Also remove the unused
output index and disabled round-robin code from choose_output, and remove a
genrecog selection whose result is discarded.  Diagnose missing or repeated
genrecog -H options, remove its fixed header-name limit, and stop printing
parsed output options to standard output.

Robin added the shared choose_output helper for PR111600, following the
size-based policy that Tamar used in genmatch.

Bootstrapped and tested on aarch64-none-linux-gnu and x86_64-linux.
Ok for trunk?
Thanks,
Kyrill

gcc/ChangeLog:

	* gensupport.h (generator_output): New structure.
	(add_generator_output, open_generator_outputs): Declare.
	(close_generator_outputs): Likewise.
	(choose_output): Accept generator_output records.  Remove the output
	index parameter.
	* gensupport.cc (add_generator_output): New function.
	(open_generator_outputs, close_generator_outputs): Likewise.
	(choose_output): Select only partition records.  Remove the output index
	and disabled round-robin implementation.
	* genemit.cc (output_files): Use generator_output records.
	(handle_arg): Register output names.
	(main): Use the common open, selection, and close helpers.
	* genrecog.cc (print_subroutine_group): Accept generator_output records.
	(header_name): Change to a pointer.
	(header): Remove global variable.
	(output_files): Use generator_output records.
	(handle_arg): Register output names and validate -H.
	(main): Validate and register the header output.  Use the common open,
	selection, and close helpers.  Remove an unused output selection.

Suggested-by: Tamar Christina <tamar.christina@arm.com>
Signed-off-by: Kyrylo Tkachov <ktkachov@nvidia.com>
---
 gcc/genemit.cc    | 25 ++++++---------
 gcc/genrecog.cc   | 64 +++++++++++++++----------------------
 gcc/gensupport.cc | 80 ++++++++++++++++++++++++++++++++++++-----------
 gcc/gensupport.h  | 16 +++++++++-
 4 files changed, 112 insertions(+), 73 deletions(-)

Comments

Robin Dapp Aug. 5, 2026, 3:22 p.m. UTC | #1 
> From: Kyrylo Tkachov <ktkachov@nvidia.com>
>
> genemit and genrecog use the same size-based output selection, but each
> generator owns its file names, opens files during option parsing, and closes
> them separately.  Adding more partitioned generators would copy this logic.
>
> Put the common output record and lifecycle helpers in gensupport.  The helpers
> validate names, delay file opening until option parsing is complete, select
> only partition outputs by current size, and close every output with a useful
> diagnostic.  Fixed outputs, such as headers, use the same record but do not
> participate in selection.
>
> Convert genemit and genrecog to the common interface.  Also remove the unused
> output index and disabled round-robin code from choose_output, and remove a
> genrecog selection whose result is discarded.  Diagnose missing or repeated
> genrecog -H options, remove its fixed header-name limit, and stop printing
> parsed output options to standard output.

This mostly reads like a more verbose changelog and doesn't really give 
a rationale.  For example: While it might be ok to rip out the non-ftell 
approach, why did you do it?  Or rather, if it's unnecessary, why not 
remove its origin in genmatch as well?  Granted, back when introducing 
this to gensupport, I didn't really verify if we need it but simply 
copied.  To my knowledge nobody ever used this particular de-feature.  
But IMHO now would be the time to check :)
Tamar Christina Aug. 5, 2026, 3:45 p.m. UTC | #2 
> -----Original Message-----
> From: Robin Dapp <rdapp.gcc@gmail.com>
> Sent: 05 August 2026 16:22
> To: ktkachov@nvidia.com; gcc-patches@gcc.gnu.org
> Cc: Tamar Christina <Tamar.Christina@arm.com>;
> jeffrey.law@oss.qualcomm.com; rdapp.gcc@gmail.com
> Subject: Re: [PATCH 1/4] gensupport: Centralize machine-generator output
> management
> 
> > From: Kyrylo Tkachov <ktkachov@nvidia.com>
> >
> > genemit and genrecog use the same size-based output selection, but each
> > generator owns its file names, opens files during option parsing, and closes
> > them separately.  Adding more partitioned generators would copy this logic.
> >
> > Put the common output record and lifecycle helpers in gensupport.  The
> helpers
> > validate names, delay file opening until option parsing is complete, select
> > only partition outputs by current size, and close every output with a useful
> > diagnostic.  Fixed outputs, such as headers, use the same record but do not
> > participate in selection.
> >
> > Convert genemit and genrecog to the common interface.  Also remove the
> unused
> > output index and disabled round-robin code from choose_output, and
> remove a
> > genrecog selection whose result is discarded.  Diagnose missing or repeated
> > genrecog -H options, remove its fixed header-name limit, and stop printing
> > parsed output options to standard output.
> 
> This mostly reads like a more verbose changelog and doesn't really give
> a rationale.  For example: While it might be ok to rip out the non-ftell
> approach, why did you do it?  Or rather, if it's unnecessary, why not
> remove its origin in genmatch as well?  Granted, back when introducing
> this to gensupport, I didn't really verify if we need it but simply
> copied.  To my knowledge nobody ever used this particular de-feature.
> But IMHO now would be the time to check :)

FWIW, the original reason for having this in genmatch was because we didn't
know how the ftell version would work out in practice so added a round-robin
approach.  It was also useful to have somewhat deterministic output for testing.
But that's less of a concern since you can just change the split count to 1.

ftell is however not guaranteed to be fast, the C standard makes no performance
guarantees on this. However more I/O managers when you call ftell on a file you've
open do not perform a seek. It can return the pointer from their internal buffers.

ftell can also fail on non-seekable streams, (pipes, sockets, etc).

on some implementations ftell can even call lseek which may trigger a kernel operation
rather than being fully user-mode like glibc normally is with buffered streams.

So while ftell is likely to be cheap, and likely to work, we left a workaround for when it doesn't.

This is of course separate from the question of whether the workaround is actually needed :)

Hopefully that clarifies the original intent.

Thanks,
Tamar

> 
> --
> Regards
>  Robin
diff mbox series

Patch

diff --git a/gcc/genemit.cc b/gcc/genemit.cc
index 6323aeb19fc..4cddaaa59b9 100644
--- a/gcc/genemit.cc
+++ b/gcc/genemit.cc
@@ -878,15 +878,14 @@  from the machine description file `md'.  */\n\n");
   fprintf (file, "#include \"target.h\"\n\n");
 }
 
-auto_vec<FILE *, 10> output_files;
+auto_vec<generator_output, 10> output_files;
 
 static bool
 handle_arg (const char *arg)
 {
   if (arg[1] == 'O')
     {
-      FILE *file = fopen (&arg[2], "w");
-      output_files.safe_push (file);
+      add_generator_output (output_files, &arg[2], true);
       return true;
     }
   return false;
@@ -910,13 +909,13 @@  main (int argc, const char **argv)
   md_rtx_info info;
 
   if (output_files.is_empty ())
-    output_files.safe_push (stdout);
+    add_generator_output (output_files, NULL, true);
+  open_generator_outputs (output_files);
 
-  for (auto f : output_files)
-    print_header (f);
+  for (const generator_output &output : output_files)
+    print_header (output.file);
 
   FILE *file = NULL;
-  unsigned file_idx;
 
   /* Read the machine description.  */
   while (read_md_rtx (&info))
@@ -941,7 +940,7 @@  main (int argc, const char **argv)
 
   for (auto &info : queue)
     {
-      file = choose_output (output_files, file_idx);
+      file = choose_output (output_files);
 
       fprintf (file, "/* %s:%d */\n", info.loc.filename, info.loc.lineno);
       switch (GET_CODE (info.def))
@@ -964,7 +963,7 @@  main (int argc, const char **argv)
 	}
     }
 
-  file = choose_output (output_files, file_idx);
+  file = choose_output (output_files);
 
   /* Write out the routines to add CLOBBERs to a pattern and say whether they
      clobber a hard reg.  */
@@ -978,10 +977,6 @@  main (int argc, const char **argv)
       handle_overloaded_gen (oname, file);
     }
 
-  int ret = SUCCESS_EXIT_CODE;
-  for (FILE *f : output_files)
-    if (fclose (f) != 0)
-      ret = FATAL_EXIT_CODE;
-
-  return ret;
+  return (close_generator_outputs (output_files)
+	  ? SUCCESS_EXIT_CODE : FATAL_EXIT_CODE);
 }
diff --git a/gcc/genrecog.cc b/gcc/genrecog.cc
index 663620224ab..ab9f3723e89 100644
--- a/gcc/genrecog.cc
+++ b/gcc/genrecog.cc
@@ -5288,11 +5288,10 @@  print_subroutine (FILE *f, output_state *os, state *s, int proc_id,
 /* Print out a routine of type TYPE that performs ROOT.  */
 
 static void
-print_subroutine_group (vec<FILE *> &vec, FILE *header, output_state *os,
+print_subroutine_group (const vec<generator_output> &outputs, FILE *header,
+			output_state *os,
 			routine_type type, state *root)
 {
-  FILE *f;
-  unsigned idx;
   os->type = type;
   if (use_subroutines_p)
     {
@@ -5305,19 +5304,14 @@  print_subroutine_group (vec<FILE *> &vec, FILE *header, output_state *os,
       unsigned int i;
       state *s;
 
-      FILE *f = header;
       FOR_EACH_VEC_ELT (subroutines, i, s)
 	print_subroutine (header, os, s, i + 1, true);
 
       FOR_EACH_VEC_ELT (subroutines, i, s)
-	{
-	  f = choose_output (vec, idx);
-	  print_subroutine (f, os, s, i + 1);
-	}
+	print_subroutine (choose_output (outputs), os, s, i + 1);
     }
   /* Output the main routine.  */
-  f = choose_output (vec, idx);
-  print_subroutine (f, os, root, 0);
+  print_subroutine (choose_output (outputs), os, root, 0);
 }
 
 /* Return the rtx pattern for the list of rtxes in a define_peephole2.  */
@@ -5388,24 +5382,22 @@  remove_clobbers (acceptance_type *acceptance_ptr, rtx *pattern_ptr)
   return true;
 }
 
-auto_vec<FILE *, 10> output_files;
-char header_name[255];
-FILE *header = NULL;
+auto_vec<generator_output, 10> output_files;
+const char *header_name;
 
 static bool
 handle_arg (const char *arg)
 {
-  printf ("%s\n", arg);
   if (arg[1] == 'O')
     {
-      FILE *file = fopen (&arg[2], "w");
-      output_files.safe_push (file);
+      add_generator_output (output_files, &arg[2], true);
       return true;
     }
   if (arg[1] == 'H')
     {
-      snprintf (header_name, 255, "%s", &arg[2]);
-      header = fopen (header_name, "w");
+      if (header_name)
+	fatal ("option -H specified more than once");
+      header_name = &arg[2];
       return true;
     }
   return false;
@@ -5421,14 +5413,18 @@  main (int argc, const char **argv)
   if (!init_rtx_reader_args_cb (argc, argv, handle_arg))
     return (FATAL_EXIT_CODE);
 
+  if (!header_name)
+    fatal ("no -H output file specified");
   if (output_files.is_empty ())
-    output_files.safe_push (stdout);
-
-  for (auto f : output_files)
-    write_header (f, header_name);
+    add_generator_output (output_files, NULL, true);
+  unsigned int header_index
+    = add_generator_output (output_files, header_name, false);
+  open_generator_outputs (output_files);
+  FILE *header = output_files[header_index].file;
 
-  FILE *file = NULL;
-  unsigned file_idx;
+  for (const generator_output &output : output_files)
+    if (output.partition_p)
+      write_header (output.file, header_name);
 
   /* Read the machine description.  */
 
@@ -5436,7 +5432,6 @@  main (int argc, const char **argv)
   while (read_md_rtx (&info))
     {
       rtx def = info.def;
-      file = choose_output (output_files, file_idx);
 
       acceptance_type acceptance;
       acceptance.partial_p = false;
@@ -5494,8 +5489,9 @@  main (int argc, const char **argv)
   if (have_error)
     return FATAL_EXIT_CODE;
 
-  for (auto f : output_files)
-    fprintf (f, "%s", "\n\n");
+  for (const generator_output &output : output_files)
+    if (output.partition_p)
+      fprintf (output.file, "%s", "\n\n");
 
   /* Optimize each routine in turn.  */
   optimize_subroutine_group ("recog", &insn_root);
@@ -5522,10 +5518,7 @@  main (int argc, const char **argv)
 	print_pattern (header, &os, routine, true);
 
       FOR_EACH_VEC_ELT (patterns, i, routine)
-	{
-	  file = choose_output (output_files, file_idx);
-	  print_pattern (file, &os, routine);
-	}
+	print_pattern (choose_output (output_files), &os, routine);
     }
 
   /* Print out the matching routines.  */
@@ -5533,11 +5526,6 @@  main (int argc, const char **argv)
   print_subroutine_group (output_files, header, &os, SPLIT, &split_root);
   print_subroutine_group (output_files, header, &os, PEEPHOLE2, &peephole2_root);
 
-  fclose (header);
-
-  int ret = SUCCESS_EXIT_CODE;
-  for (FILE *f : output_files)
-    if (fclose (f) != 0)
-      ret = FATAL_EXIT_CODE;
-  return ret;
+  return (close_generator_outputs (output_files)
+	  ? SUCCESS_EXIT_CODE : FATAL_EXIT_CODE);
 }
diff --git a/gcc/gensupport.cc b/gcc/gensupport.cc
index 5fcc30d340e..9439ddd97d8 100644
--- a/gcc/gensupport.cc
+++ b/gcc/gensupport.cc
@@ -3935,35 +3935,77 @@  find_optab (optab_pattern *p, const char *name)
   return false;
 }
 
-/* Find the file to write into next.  We try to evenly distribute the contents
-   over the different files.  */
+/* Add output NAME to OUTPUTS.  A null NAME means standard output.
+   PARTITION_P is true if the output participates in size-based selection.
+   Return its index.  */
 
-#define SIZED_BASED_CHUNKS 1
+unsigned int
+add_generator_output (vec<generator_output> &outputs, const char *name,
+		      bool partition_p)
+{
+  gcc_assert (name || outputs.is_empty ());
+  if (name)
+    for (const generator_output &output : outputs)
+      if (output.name && canonical_filename_eq (name, output.name))
+	fatal ("output file %s specified more than once", name);
+
+  generator_output output = { name, name ? NULL : stdout, partition_p };
+  unsigned int index = outputs.length ();
+  outputs.safe_push (output);
+  return index;
+}
+
+/* Open each named file in OUTPUTS.  */
+
+void
+open_generator_outputs (vec<generator_output> &outputs)
+{
+  for (generator_output &output : outputs)
+    if (!output.file)
+      {
+	output.file = fopen (output.name, "w");
+	if (!output.file)
+	  fatal ("cannot open file %s: %s", output.name, xstrerror (errno));
+      }
+}
+
+/* Return the shortest partition file in OUTPUTS.  */
 
 FILE *
-choose_output (const vec<FILE *> &parts, unsigned &idx)
+choose_output (const vec<generator_output> &outputs)
 {
-  if (parts.length () == 0)
-    gcc_unreachable ();
-#ifdef SIZED_BASED_CHUNKS
   FILE *shortest = NULL;
   long min = 0;
-  idx = 0;
-  for (unsigned i = 0; i < parts.length (); i++)
+  for (const generator_output &output : outputs)
     {
-      FILE *part  = parts[i];
-      long len = ftell (part);
+      if (!output.partition_p)
+	continue;
+      long len = ftell (output.file);
       if (!shortest || min > len)
 	{
-	  shortest = part;
+	  shortest = output.file;
 	  min = len;
-	  idx = i;
-       }
+	}
     }
+  if (!shortest)
+    gcc_unreachable ();
   return shortest;
-#else
-  static int current_file;
-  idx = current_file++ % parts.length ();
-  return parts[idx];
-#endif
+}
+
+/* Close all files in OUTPUTS.  Return true if every close succeeds.  */
+
+bool
+close_generator_outputs (const vec<generator_output> &outputs)
+{
+  bool ok = true;
+  for (const generator_output &output : outputs)
+    {
+      if (fclose (output.file) != 0)
+	{
+	  error ("cannot close output %s: %s",
+		 output.name ? output.name : "<stdout>", xstrerror (errno));
+	  ok = false;
+	}
+    }
+  return ok;
 }
diff --git a/gcc/gensupport.h b/gcc/gensupport.h
index 86dd1103436..880505355b2 100644
--- a/gcc/gensupport.h
+++ b/gcc/gensupport.h
@@ -232,6 +232,20 @@  extern void compute_test_codes (rtx, file_location, char *);
 extern file_location get_file_location (rtx);
 extern const char *get_emit_function (rtx);
 extern bool find_optab (optab_pattern *, const char *);
-extern FILE *choose_output (const vec<FILE *> &, unsigned &);
+
+/* An output file produced by a machine-description generator.  Partition
+   files participate in size-based output selection.  */
+struct generator_output
+{
+  const char *name;
+  FILE *file;
+  bool partition_p;
+};
+
+extern unsigned int add_generator_output (vec<generator_output> &,
+					  const char *, bool);
+extern void open_generator_outputs (vec<generator_output> &);
+extern FILE *choose_output (const vec<generator_output> &);
+extern bool close_generator_outputs (const vec<generator_output> &);
 
 #endif /* GCC_GENSUPPORT_H */