Adjust LTO mode tables for "Machine_Mode: Extend machine_mode from 8 to 16 bits" (was: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits)

[Thomas Schwinge <thomas@codesourcery.com>]

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

Hi!

On 2023-05-13T16:44:41+0800, Kito Cheng via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> Tried this patch and I ran into some issues, some variables are using
> unsigned char to hold machine mode and will have problems when the
> number of modes is larger than 255...
>
> And here is the fix:

> --- a/gcc/genmodes.cc
> +++ b/gcc/genmodes.cc
> @@ -1141,10 +1141,10 @@ inline __attribute__((__always_inline__))\n\
> #else\n\
> extern __inline__ __attribute__((__always_inline__, __gnu_inline__))\n\
> #endif\n\
> -unsigned char\n\
> +unsigned short\n\
> mode_inner_inline (machine_mode mode)\n\
> {\n\
> -  extern const unsigned char mode_inner[NUM_MACHINE_MODES];\n\
> +  extern const unsigned short mode_inner[NUM_MACHINE_MODES];\n\
>   gcc_assert (mode >= 0 && mode < NUM_MACHINE_MODES);\n\
>   switch (mode)\n\
>     {");
> @@ -1529,7 +1529,7 @@ emit_mode_wider (void)
>   int c;
>   struct mode_data *m;
>
> -  print_decl ("unsigned char", "mode_next", "NUM_MACHINE_MODES");
> +  print_decl ("unsigned short", "mode_next", "NUM_MACHINE_MODES");

Etc.

Instead of 's%char%short', shouldn't we really be using
'enum machine_mode' here?  (I understand such a change may require some
further surgery, but wouldn't it be the correct thing to do?)


And, in context of working on
<https://inbox.sourceware.org/87mt0hcp12.fsf@euler.schwinge.homeip.net>
"Streamer: Fix out of range memory access of machine mode", I found
another one, see attached
'[WIP] Adjust LTO mode tables for "Machine_Mode: Extend machine_mode from 8 to 16 bits"'
(..., which applies on top of the former.)  There, in fact, I did change
to 'enum machine_mode' instead of 's%char%short' -- correct?  Any
comments on the 'GTY' issues: (1) 'const' build error,
(2) '[build-gcc]/gcc/gtype-desc.cc' changes, and (3) is 'GTY((atomic))'
actually the right thing to use, here?

In particular, the 'lto_mode_identity_table' changes would seem necessary
to keep standard LTO ('-flto') functional for large 'machine_mode' size?


Bernhard: Fancy writing a Coccinelle check whether there are any more
places where we put what originally was a 'machine_mode' type into a
'char' (or, into a non-'machine_mode' generally)?  ;-) Hey, just a Friday
afternoon idea!


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Adjust-LTO-mode-tables-for-Machine_Mode-Extend-m.patch --]
[-- Type: text/x-diff, Size: 6119 bytes --]

From 0fd8f65bb87b11ef8ae366a797aec572d67b284f Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Fri, 30 Jun 2023 13:23:55 +0200
Subject: [PATCH] [WIP] Adjust LTO mode tables for "Machine_Mode: Extend
 machine_mode from 8 to 16 bits"

---
 gcc/lto-streamer-in.cc |  2 +-
 gcc/lto-streamer.h     | 56 +++++++++++++++++++++++++++++++++++++++++-
 gcc/lto/lto-common.cc  | 10 ++++----
 gcc/lto/lto-common.h   |  2 +-
 gcc/tree-streamer.h    |  2 +-
 5 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc
index 1876e1967ec..bbd44504ff8 100644
--- a/gcc/lto-streamer-in.cc
+++ b/gcc/lto-streamer-in.cc
@@ -1997,7 +1997,7 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
   bitpack_d bp = streamer_read_bitpack (&ib);
 
   unsigned mode_bits = bp_unpack_value (&bp, 5);
-  unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits);
+  machine_mode *table = ggc_cleared_vec_alloc<machine_mode> (1 << mode_bits);
 
   file_data->mode_table = table;
   file_data->mode_bits = mode_bits;
diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index 0556b34c837..4d83741e4c6 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -596,7 +596,61 @@ struct GTY(()) lto_file_decl_data
   hash_map<tree, ld_plugin_symbol_resolution> * GTY((skip)) resolution_map;
 
   /* Mode translation table.  */
-  const unsigned char *mode_table;
+  /*TODO const
+With 'const', we get:
+
+    gtype-desc.cc: In function 'void gt_pch_nx_lto_file_decl_data(void*)':
+    gtype-desc.cc:6531:34: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
+             gt_pch_note_object ((*x).mode_table, x, gt_pch_p_18lto_file_decl_data);
+                                      ^
+    In file included from [...]/source-gcc/gcc/hash-table.h:247:0,
+                     from [...]/source-gcc/gcc/coretypes.h:486,
+                     from gtype-desc.cc:23:
+    [...]/source-gcc/gcc/ggc.h:47:12: note:   initializing argument 1 of 'int gt_pch_note_object(void*, void*, gt_note_pointers, size_t)'
+     extern int gt_pch_note_object (void *, void *, gt_note_pointers,
+                ^
+    make[2]: *** [Makefile:1180: gtype-desc.o] Error 1
+   */
+  machine_mode * GTY((atomic)) mode_table;
+  /*
+This (without 'const') changes '[build-gcc]/gcc/gtype-desc.cc' as follows:
+
+    @@ -2566,7 +2566,9 @@ gt_ggc_mx_lto_file_decl_data (void *x_p)
+           gt_ggc_m_17lto_in_decl_state ((*x).global_decl_state);
+           gt_ggc_m_29hash_table_decl_state_hasher_ ((*x).function_decl_states);
+           gt_ggc_m_18lto_file_decl_data ((*x).next);
+    -      gt_ggc_m_S ((*x).mode_table);
+    +      if ((*x).mode_table != NULL) {
+    +        ggc_mark ((*x).mode_table);
+    +      }
+         }
+     }
+     
+    @@ -6525,7 +6527,9 @@ gt_pch_nx_lto_file_decl_data (void *x_p)
+           gt_pch_n_17lto_in_decl_state ((*x).global_decl_state);
+           gt_pch_n_29hash_table_decl_state_hasher_ ((*x).function_decl_states);
+           gt_pch_n_18lto_file_decl_data ((*x).next);
+    -      gt_pch_n_S ((*x).mode_table);
+    +      if ((*x).mode_table != NULL) {
+    +        gt_pch_note_object ((*x).mode_table, x, gt_pch_p_18lto_file_decl_data);
+    +      }
+         }
+     }
+     
+    @@ -10929,8 +10933,10 @@ gt_pch_p_18lto_file_decl_data (ATTRIBUTE
+         op (&((*x).function_decl_states), NULL, cookie);
+       if ((void *)(x) == this_obj)
+         op (&((*x).next), NULL, cookie);
+    -  if ((void *)(x) == this_obj)
+    -    op (&((*x).mode_table), NULL, cookie);
+    +  if ((*x).mode_table != NULL) {
+    +    if ((void *)(x) == this_obj)
+    +      op (&((*x).mode_table), NULL, cookie);
+    +  }
+     }
+
+Given that the '[...]_S' routines are for strings (due to previous 'char *', I suppose), that's probably correct?
+   */
 
   /* Read LTO section.  */
   lto_section lto_section_header;
diff --git a/gcc/lto/lto-common.cc b/gcc/lto/lto-common.cc
index 973ab791712..f483f42997e 100644
--- a/gcc/lto/lto-common.cc
+++ b/gcc/lto/lto-common.cc
@@ -64,7 +64,7 @@ static bool type_streaming_finished = false;
 
 GTY(()) tree first_personality_decl;
 
-GTY(()) const unsigned char *lto_mode_identity_table;
+GTY(()) const machine_mode *lto_mode_identity_table;
 
 /* Returns a hash code for P.  */
 
@@ -2274,7 +2274,7 @@ lto_file_finalize (struct lto_file_decl_data *file_data, lto_file *file,
 #ifdef ACCEL_COMPILER
   lto_input_mode_table (file_data);
 #else
-  file_data->mode_table = lto_mode_identity_table;
+  file_data->mode_table = /*TODO const*/ (machine_mode *) lto_mode_identity_table;
   file_data->mode_bits = ceil_log2 (MAX_MACHINE_MODE);
 #endif
 
@@ -3116,10 +3116,10 @@ lto_fe_init (void)
   bitmap_obstack_initialize (NULL);
   gimple_register_cfg_hooks ();
 #ifndef ACCEL_COMPILER
-  unsigned char *table
-    = ggc_vec_alloc<unsigned char> (MAX_MACHINE_MODE);
+  machine_mode *table
+    = ggc_vec_alloc<machine_mode> (MAX_MACHINE_MODE);
   for (int m = 0; m < MAX_MACHINE_MODE; m++)
-    table[m] = m;
+    table[m] = (machine_mode) m;
   lto_mode_identity_table = table;
 #endif
 }
diff --git a/gcc/lto/lto-common.h b/gcc/lto/lto-common.h
index 24b2445673b..2b868085139 100644
--- a/gcc/lto/lto-common.h
+++ b/gcc/lto/lto-common.h
@@ -26,7 +26,7 @@ void print_lto_report_1 (void);
 
 extern tree lto_eh_personality_decl;
 extern GTY(()) vec<tree, va_gc> *tree_with_vars;
-extern const unsigned char *lto_mode_identity_table;
+extern const machine_mode *lto_mode_identity_table;
 extern tree first_personality_decl;
 
 #endif
diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h
index ff49d1ba637..1e346b775e6 100644
--- a/gcc/tree-streamer.h
+++ b/gcc/tree-streamer.h
@@ -118,7 +118,7 @@ bp_unpack_machine_mode (struct bitpack_d *bp)
   lto_input_block *ib = (class lto_input_block *) bp->stream;
   int last = 1 << ib->file_data->mode_bits;
   unsigned ix = bp_unpack_enum (bp, machine_mode, last);
-  return (machine_mode) ib->file_data->mode_table[ix];
+  return ib->file_data->mode_table[ix];
 }
 
 #endif  /* GCC_TREE_STREAMER_H  */
-- 
2.34.1