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....@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
>From 0fd8f65bb87b11ef8ae366a797aec572d67b284f Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@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
