Machine_Mode: Extend machine_mode from 8 to 16 bits

Message ID	20230512050016.476110-1-pan2.li@intel.com
State	New
Headers	show Return-Path: <gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org> DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6A52E3858C5E To: gcc-patches@gcc.gnu.org Cc: juzhe.zhong@rivai.ai, kito.cheng@sifive.com, pan2.li@intel.com, yanzhang.wang@intel.com, jeffreyalaw@gmail.com, rguenther@suse.de, richard.sandiford@arm.com Subject: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits Date: Fri, 12 May 2023 13:00:16 +0800 Message-Id: <20230512050016.476110-1-pan2.li@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list From: Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: pan2.li@intel.com Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org>
Series	Machine_Mode: Extend machine_mode from 8 to 16 bits \| expand Machine_Mode: Extend machine_mode from 8 to 16 bits

Li, Pan2 via Gcc-patches May 12, 2023, 5 a.m. UTC

From: Pan Li <pan2.li@intel.com>

We are running out of the machine_mode(8 bits) in RISC-V backend. Thus
we would like to extend the machine mode bit size from 8 to 16 bits.
However, it is sensitive to extend the memory size in common structure
like tree or rtx. This patch would like to extend the machine mode bits
to 16 bits by shrinking, like:

* Swap the bit size of code and machine code in rtx_def.
* Reconcile the machine_mode location and spare in tree.

The memory impact of this patch for correlated structure looks like below:

+-------------------+----------+---------+------+
| struct/bytes      | upstream | patched | diff |
+-------------------+----------+---------+------+
| rtx_obj_reference |        8 |      12 |   +4 |
| ext_modified      |        2 |       3 |   +1 |
| ira_allocno       |      192 |     200 |   +8 |
| qty_table_elem    |       40 |      40 |    0 |
| reg_stat_type     |       64 |      64 |    0 |
| rtx_def           |       40 |      40 |    0 |
| table_elt         |       80 |      80 |    0 |
| tree_decl_common  |      112 |     112 |    0 |
| tree_type_common  |      128 |     128 |    0 |
+-------------------+----------+---------+------+

The tree and rtx related struct has no memory changes after this patch,
and the machine_mode changes to 16 bits already.

Signed-off-by: Pan Li <pan2.li@intel.com>
Co-authored-by: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
Co-authored-by: Kito Cheng <kito.cheng@sifive.com>

gcc/ChangeLog:

	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
	* cse.cc (struct qty_table_elem): Ditto.
	(struct table_elt): Ditto.
	(struct set): Ditto.
	* genopinit.cc (main): Reconciled the machine mode limit.
	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.
	* rtl-ssa/accesses.h: Ditto.
	* rtl.h (RTX_CODE_BITSIZE): New macro.
	(RTX_MACHINE_MODE_BITSIZE): Ditto.
	(struct GTY): Swap bit size between code and machine mode.
	(subreg_shape::unique_id): Reconciled the machine mode limit.
	* rtlanal.h: Extended machine mode to 16 bits.
	* tree-core.h (struct tree_type_common): Ditto.
	(struct tree_decl_common): Reconciled the locate and extended
	bit size of machine mode.
---
 gcc/combine.cc         |  4 ++--
 gcc/cse.cc             |  8 ++++----
 gcc/genopinit.cc       |  3 ++-
 gcc/ira-int.h          | 12 ++++++++----
 gcc/ree.cc             |  2 +-
 gcc/rtl-ssa/accesses.h |  6 ++++--
 gcc/rtl.h              |  9 ++++++---
 gcc/rtlanal.h          |  5 +++--
 gcc/tree-core.h        | 11 ++++++++---
 9 files changed, 38 insertions(+), 22 deletions(-)

Richard Biener May 12, 2023, 6:49 a.m. UTC | #1

On Fri, 12 May 2023, pan2.li@intel.com wrote:

> From: Pan Li <pan2.li@intel.com>
> 
> We are running out of the machine_mode(8 bits) in RISC-V backend. Thus
> we would like to extend the machine mode bit size from 8 to 16 bits.
> However, it is sensitive to extend the memory size in common structure
> like tree or rtx. This patch would like to extend the machine mode bits
> to 16 bits by shrinking, like:
> 
> * Swap the bit size of code and machine code in rtx_def.
> * Reconcile the machine_mode location and spare in tree.
> 
> The memory impact of this patch for correlated structure looks like below:
> 
> +-------------------+----------+---------+------+
> | struct/bytes      | upstream | patched | diff |
> +-------------------+----------+---------+------+
> | rtx_obj_reference |        8 |      12 |   +4 |
> | ext_modified      |        2 |       3 |   +1 |

this struct is packed and we have an array of it - it _might_ be
bad to have elements of size 3 here.  Size 4 shouldn't be too
bad so I suggest to remove the packed attribute there.

> | ira_allocno       |      192 |     200 |   +8 |

that looks unfortunate - did you check if there's now
padding that could be used by re-ordering fields?

> | qty_table_elem    |       40 |      40 |    0 |
> | reg_stat_type     |       64 |      64 |    0 |
> | rtx_def           |       40 |      40 |    0 |
> | table_elt         |       80 |      80 |    0 |
> | tree_decl_common  |      112 |     112 |    0 |
> | tree_type_common  |      128 |     128 |    0 |
> +-------------------+----------+---------+------+
> 
> The tree and rtx related struct has no memory changes after this patch,
> and the machine_mode changes to 16 bits already.
> 
> Signed-off-by: Pan Li <pan2.li@intel.com>
> Co-authored-by: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
> Co-authored-by: Kito Cheng <kito.cheng@sifive.com>
> 
> gcc/ChangeLog:
> 
> 	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
> 	* cse.cc (struct qty_table_elem): Ditto.
> 	(struct table_elt): Ditto.
> 	(struct set): Ditto.
> 	* genopinit.cc (main): Reconciled the machine mode limit.
> 	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
> 	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.

please go over the ChangeLog and properly specify the structure types
altered.  The script generating the changelog isn't perfect.

Richard.

> 	* rtl-ssa/accesses.h: Ditto.
> 	* rtl.h (RTX_CODE_BITSIZE): New macro.
> 	(RTX_MACHINE_MODE_BITSIZE): Ditto.
> 	(struct GTY): Swap bit size between code and machine mode.
> 	(subreg_shape::unique_id): Reconciled the machine mode limit.
> 	* rtlanal.h: Extended machine mode to 16 bits.
> 	* tree-core.h (struct tree_type_common): Ditto.
> 	(struct tree_decl_common): Reconciled the locate and extended
> 	bit size of machine mode.
> ---
>  gcc/combine.cc         |  4 ++--
>  gcc/cse.cc             |  8 ++++----
>  gcc/genopinit.cc       |  3 ++-
>  gcc/ira-int.h          | 12 ++++++++----
>  gcc/ree.cc             |  2 +-
>  gcc/rtl-ssa/accesses.h |  6 ++++--
>  gcc/rtl.h              |  9 ++++++---
>  gcc/rtlanal.h          |  5 +++--
>  gcc/tree-core.h        | 11 ++++++++---
>  9 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/gcc/combine.cc b/gcc/combine.cc
> index 5aa0ec5c45a..bdf6f635c80 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -200,7 +200,7 @@ struct reg_stat_type {
>  
>    unsigned HOST_WIDE_INT	last_set_nonzero_bits;
>    char				last_set_sign_bit_copies;
> -  ENUM_BITFIELD(machine_mode)	last_set_mode : 8;
> +  ENUM_BITFIELD(machine_mode)	last_set_mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* Set nonzero if references to register n in expressions should not be
>       used.  last_set_invalid is set nonzero when this register is being
> @@ -235,7 +235,7 @@ struct reg_stat_type {
>       truncation if we know that value already contains a truncated
>       value.  */
>  
> -  ENUM_BITFIELD(machine_mode)	truncated_to_mode : 8;
> +  ENUM_BITFIELD(machine_mode)	truncated_to_mode : RTX_MACHINE_MODE_BITSIZE;
>  };
>  
>  
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index b10c9b0c94d..fe594c1bc3d 100644
> --- a/gcc/cse.cc
> +++ b/gcc/cse.cc
> @@ -250,8 +250,8 @@ struct qty_table_elem
>    unsigned int first_reg, last_reg;
>    /* The sizes of these fields should match the sizes of the
>       code and mode fields of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(rtx_code) comparison_code : 16;
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(rtx_code) comparison_code : RTX_CODE_BITSIZE;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>  };
>  
>  /* The table of all qtys, indexed by qty number.  */
> @@ -406,7 +406,7 @@ struct table_elt
>    int regcost;
>    /* The size of this field should match the size
>       of the mode field of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>    char in_memory;
>    char is_const;
>    char flag;
> @@ -4155,7 +4155,7 @@ struct set
>    /* Original machine mode, in case it becomes a CONST_INT.
>       The size of this field should match the size of the mode
>       field of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>    /* Hash value of constant equivalent for SET_SRC.  */
>    unsigned src_const_hash;
>    /* A constant equivalent for SET_SRC, if any.  */
> diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
> index 83cb7504fa1..2add8b925da 100644
> --- a/gcc/genopinit.cc
> +++ b/gcc/genopinit.cc
> @@ -182,7 +182,8 @@ main (int argc, const char **argv)
>  
>    progname = "genopinit";
>  
> -  if (NUM_OPTABS > 0xffff || MAX_MACHINE_MODE >= 0xff)
> +  if (NUM_OPTABS > 0xffff
> +    || MAX_MACHINE_MODE >= ((1 << RTX_MACHINE_MODE_BITSIZE) - 1))
>      fatal ("genopinit range assumptions invalid");
>  
>    if (!init_rtx_reader_args_cb (argc, argv, handle_arg))
> diff --git a/gcc/ira-int.h b/gcc/ira-int.h
> index e2de47213b4..124e14200b1 100644
> --- a/gcc/ira-int.h
> +++ b/gcc/ira-int.h
> @@ -280,11 +280,15 @@ struct ira_allocno
>    /* Regno for allocno or cap.  */
>    int regno;
>    /* Mode of the allocno which is the mode of the corresponding
> -     pseudo-register.  */
> -  ENUM_BITFIELD (machine_mode) mode : 8;
> +     pseudo-register.  Note the bitsize of mode should be exactly
> +     the same as the definition of rtx_def, aka RTX_MACHINE_MODE_BITSIZE
> +     in rtl.h.  */
> +  ENUM_BITFIELD (machine_mode) mode : 16;
>    /* Widest mode of the allocno which in at least one case could be
> -     for paradoxical subregs where wmode > mode.  */
> -  ENUM_BITFIELD (machine_mode) wmode : 8;
> +     for paradoxical subregs where wmode > mode.  Note the bitsize of
> +     wmode should be exactly the same as the definition of rtx_def,
> +     aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */
> +  ENUM_BITFIELD (machine_mode) wmode : 16;
>    /* Register class which should be used for allocation for given
>       allocno.  NO_REGS means that we should use memory.  */
>    ENUM_BITFIELD (reg_class) aclass : 16;
> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index 413aec7c8eb..fb011bc907c 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -567,7 +567,7 @@ enum ext_modified_kind
>  struct ATTRIBUTE_PACKED ext_modified
>  {
>    /* Mode from which ree has zero or sign extended the destination.  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* Kind of modification of the insn.  */
>    ENUM_BITFIELD(ext_modified_kind) kind : 2;
> diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h
> index c5180b9308a..2e73004cafa 100644
> --- a/gcc/rtl-ssa/accesses.h
> +++ b/gcc/rtl-ssa/accesses.h
> @@ -253,8 +253,10 @@ private:
>    // Bits for future expansion.
>    unsigned int m_spare : 2;
>  
> -  // The value returned by the accessor above.
> -  machine_mode m_mode : 8;
> +  // The value returned by the accessor above.  Note the bitsize of
> +  // m_mode should be exactly the same as the definition of rtx_def,
> +  // aka RTX_MACHINE_MODE_BITSIZE in rtl.h.
> +  machine_mode m_mode : 16;
>  };
>  
>  // A contiguous array of access_info pointers.  Used to represent a
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index f634cab730b..a18ecf7632f 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -63,6 +63,9 @@ enum rtx_code  {
>  # define NON_GENERATOR_NUM_RTX_CODE ((int) MATCH_OPERAND)
>  #endif
>  
> +#define RTX_CODE_BITSIZE 8
> +#define RTX_MACHINE_MODE_BITSIZE 16
> +
>  /* Register Transfer Language EXPRESSIONS CODE CLASSES */
>  
>  enum rtx_class  {
> @@ -310,10 +313,10 @@ struct GTY((desc("0"), tag("0"),
>  	    chain_next ("RTX_NEXT (&%h)"),
>  	    chain_prev ("RTX_PREV (&%h)"))) rtx_def {
>    /* The kind of expression this is.  */
> -  ENUM_BITFIELD(rtx_code) code: 16;
> +  ENUM_BITFIELD(rtx_code) code: RTX_CODE_BITSIZE;
>  
>    /* The kind of value the expression has.  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* 1 in a MEM if we should keep the alias set for this mem unchanged
>       when we access a component.
> @@ -2164,7 +2167,7 @@ subreg_shape::operator != (const subreg_shape &other) const
>  inline unsigned HOST_WIDE_INT
>  subreg_shape::unique_id () const
>  {
> -  { STATIC_ASSERT (MAX_MACHINE_MODE <= 256); }
> +  { STATIC_ASSERT (MAX_MACHINE_MODE <= (1 << RTX_MACHINE_MODE_BITSIZE)); }
>    { STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 3); }
>    { STATIC_ASSERT (sizeof (offset.coeffs[0]) <= 2); }
>    int res = (int) inner_mode + ((int) outer_mode << 8);
> diff --git a/gcc/rtlanal.h b/gcc/rtlanal.h
> index 5fbed816e20..15aba0dec7a 100644
> --- a/gcc/rtlanal.h
> +++ b/gcc/rtlanal.h
> @@ -99,8 +99,9 @@ public:
>    unsigned int flags : 16;
>  
>    /* The mode of the reference.  If IS_MULTIREG, this is the mode of
> -     REGNO - MULTIREG_OFFSET.  */
> -  machine_mode mode : 8;
> +     REGNO - MULTIREG_OFFSET.  Note the bitsize of mode should be exactly
> +     the same as the definition of rtx_def,  */
> +  machine_mode mode : 16;
>  
>    /* If IS_MULTIREG, the offset of REGNO from the start of the register.  */
>    unsigned int multireg_offset : 8;
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index a1aea136e75..001d232f433 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1680,8 +1680,11 @@ struct GTY(()) tree_type_common {
>    tree attributes;
>    unsigned int uid;
>  
> +  /* Note the bitsize of wmode should be exactly the same as the
> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */
> +  ENUM_BITFIELD(machine_mode) mode : 16;
> +
>    unsigned int precision : 16;
> -  ENUM_BITFIELD(machine_mode) mode : 8;
>    unsigned lang_flag_0 : 1;
>    unsigned lang_flag_1 : 1;
>    unsigned lang_flag_2 : 1;
> @@ -1712,7 +1715,7 @@ struct GTY(()) tree_type_common {
>    unsigned empty_flag : 1;
>    unsigned indivisible_p : 1;
>    unsigned no_named_args_stdarg_p : 1;
> -  unsigned spare : 9;
> +  unsigned spare : 1;
>  
>    alias_set_type alias_set;
>    tree pointer_to;
> @@ -1770,7 +1773,9 @@ struct GTY(()) tree_decl_common {
>    struct tree_decl_minimal common;
>    tree size;
>  
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  /* Note the bitsize of wmode should be exactly the same as the
> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */
> +  ENUM_BITFIELD(machine_mode) mode : 16;
>  
>    unsigned nonlocal_flag : 1;
>    unsigned virtual_flag : 1;
>

Richard Sandiford May 12, 2023, 8:24 a.m. UTC | #2

pan2.li@intel.com writes:
> From: Pan Li <pan2.li@intel.com>
>
> We are running out of the machine_mode(8 bits) in RISC-V backend. Thus
> we would like to extend the machine mode bit size from 8 to 16 bits.
> However, it is sensitive to extend the memory size in common structure
> like tree or rtx. This patch would like to extend the machine mode bits
> to 16 bits by shrinking, like:
>
> * Swap the bit size of code and machine code in rtx_def.
> * Reconcile the machine_mode location and spare in tree.
>
> The memory impact of this patch for correlated structure looks like below:
>
> +-------------------+----------+---------+------+
> | struct/bytes      | upstream | patched | diff |
> +-------------------+----------+---------+------+
> | rtx_obj_reference |        8 |      12 |   +4 |
> | ext_modified      |        2 |       3 |   +1 |
> | ira_allocno       |      192 |     200 |   +8 |
> | qty_table_elem    |       40 |      40 |    0 |
> | reg_stat_type     |       64 |      64 |    0 |
> | rtx_def           |       40 |      40 |    0 |
> | table_elt         |       80 |      80 |    0 |
> | tree_decl_common  |      112 |     112 |    0 |
> | tree_type_common  |      128 |     128 |    0 |
> +-------------------+----------+---------+------+
>
> The tree and rtx related struct has no memory changes after this patch,
> and the machine_mode changes to 16 bits already.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> Co-authored-by: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
> Co-authored-by: Kito Cheng <kito.cheng@sifive.com>
>
> gcc/ChangeLog:
>
> 	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
> 	* cse.cc (struct qty_table_elem): Ditto.
> 	(struct table_elt): Ditto.
> 	(struct set): Ditto.
> 	* genopinit.cc (main): Reconciled the machine mode limit.
> 	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
> 	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.
> 	* rtl-ssa/accesses.h: Ditto.
> 	* rtl.h (RTX_CODE_BITSIZE): New macro.
> 	(RTX_MACHINE_MODE_BITSIZE): Ditto.
> 	(struct GTY): Swap bit size between code and machine mode.
> 	(subreg_shape::unique_id): Reconciled the machine mode limit.
> 	* rtlanal.h: Extended machine mode to 16 bits.
> 	* tree-core.h (struct tree_type_common): Ditto.
> 	(struct tree_decl_common): Reconciled the locate and extended
> 	bit size of machine mode.
> ---
>  gcc/combine.cc         |  4 ++--
>  gcc/cse.cc             |  8 ++++----
>  gcc/genopinit.cc       |  3 ++-
>  gcc/ira-int.h          | 12 ++++++++----
>  gcc/ree.cc             |  2 +-
>  gcc/rtl-ssa/accesses.h |  6 ++++--
>  gcc/rtl.h              |  9 ++++++---
>  gcc/rtlanal.h          |  5 +++--
>  gcc/tree-core.h        | 11 ++++++++---
>  9 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/gcc/combine.cc b/gcc/combine.cc
> index 5aa0ec5c45a..bdf6f635c80 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -200,7 +200,7 @@ struct reg_stat_type {
>  
>    unsigned HOST_WIDE_INT	last_set_nonzero_bits;
>    char				last_set_sign_bit_copies;
> -  ENUM_BITFIELD(machine_mode)	last_set_mode : 8;
> +  ENUM_BITFIELD(machine_mode)	last_set_mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* Set nonzero if references to register n in expressions should not be
>       used.  last_set_invalid is set nonzero when this register is being
> @@ -235,7 +235,7 @@ struct reg_stat_type {
>       truncation if we know that value already contains a truncated
>       value.  */
>  
> -  ENUM_BITFIELD(machine_mode)	truncated_to_mode : 8;
> +  ENUM_BITFIELD(machine_mode)	truncated_to_mode : RTX_MACHINE_MODE_BITSIZE;
>  };
>  
>  
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index b10c9b0c94d..fe594c1bc3d 100644
> --- a/gcc/cse.cc
> +++ b/gcc/cse.cc
> @@ -250,8 +250,8 @@ struct qty_table_elem
>    unsigned int first_reg, last_reg;
>    /* The sizes of these fields should match the sizes of the
>       code and mode fields of struct rtx_def (see rtl.h).  */

The comment can be removed, since you're now adding macros to ensure
this (thanks).  Same for other instances of the comment.

> -  ENUM_BITFIELD(rtx_code) comparison_code : 16;
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(rtx_code) comparison_code : RTX_CODE_BITSIZE;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;

Please put the mode first, so that the 16-bit value is aligned
to 16 bits.

>  };
>  
>  /* The table of all qtys, indexed by qty number.  */
> @@ -406,7 +406,7 @@ struct table_elt
>    int regcost;
>    /* The size of this field should match the size
>       of the mode field of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>    char in_memory;
>    char is_const;
>    char flag;
> @@ -4155,7 +4155,7 @@ struct set
>    /* Original machine mode, in case it becomes a CONST_INT.
>       The size of this field should match the size of the mode
>       field of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>    /* Hash value of constant equivalent for SET_SRC.  */
>    unsigned src_const_hash;
>    /* A constant equivalent for SET_SRC, if any.  */
> diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
> index 83cb7504fa1..2add8b925da 100644
> --- a/gcc/genopinit.cc
> +++ b/gcc/genopinit.cc
> @@ -182,7 +182,8 @@ main (int argc, const char **argv)
>  
>    progname = "genopinit";
>  
> -  if (NUM_OPTABS > 0xffff || MAX_MACHINE_MODE >= 0xff)
> +  if (NUM_OPTABS > 0xffff
> +    || MAX_MACHINE_MODE >= ((1 << RTX_MACHINE_MODE_BITSIZE) - 1))
>      fatal ("genopinit range assumptions invalid");
>  
>    if (!init_rtx_reader_args_cb (argc, argv, handle_arg))
> diff --git a/gcc/ira-int.h b/gcc/ira-int.h
> index e2de47213b4..124e14200b1 100644
> --- a/gcc/ira-int.h
> +++ b/gcc/ira-int.h
> @@ -280,11 +280,15 @@ struct ira_allocno
>    /* Regno for allocno or cap.  */
>    int regno;
>    /* Mode of the allocno which is the mode of the corresponding
> -     pseudo-register.  */
> -  ENUM_BITFIELD (machine_mode) mode : 8;
> +     pseudo-register.  Note the bitsize of mode should be exactly
> +     the same as the definition of rtx_def, aka RTX_MACHINE_MODE_BITSIZE
> +     in rtl.h.  */
> +  ENUM_BITFIELD (machine_mode) mode : 16;
>    /* Widest mode of the allocno which in at least one case could be
> -     for paradoxical subregs where wmode > mode.  */
> -  ENUM_BITFIELD (machine_mode) wmode : 8;
> +     for paradoxical subregs where wmode > mode.  Note the bitsize of
> +     wmode should be exactly the same as the definition of rtx_def,
> +     aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */
> +  ENUM_BITFIELD (machine_mode) wmode : 16;
>    /* Register class which should be used for allocation for given
>       allocno.  NO_REGS means that we should use memory.  */
>    ENUM_BITFIELD (reg_class) aclass : 16;

Why not use the BITSIZE macros directly?  Any reasonable use of
ira-int.h will also need rtl.h.  If something includes ira-int.h
before rtl.h then we should change that.

To avoid growing this structure, please move something into one of
the holes later in the structure.  E.g. hard_regno could go before
num_objects.

> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index 413aec7c8eb..fb011bc907c 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -567,7 +567,7 @@ enum ext_modified_kind
>  struct ATTRIBUTE_PACKED ext_modified
>  {
>    /* Mode from which ree has zero or sign extended the destination.  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* Kind of modification of the insn.  */
>    ENUM_BITFIELD(ext_modified_kind) kind : 2;
> diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h
> index c5180b9308a..2e73004cafa 100644
> --- a/gcc/rtl-ssa/accesses.h
> +++ b/gcc/rtl-ssa/accesses.h
> @@ -253,8 +253,10 @@ private:
>    // Bits for future expansion.
>    unsigned int m_spare : 2;
>  
> -  // The value returned by the accessor above.
> -  machine_mode m_mode : 8;
> +  // The value returned by the accessor above.  Note the bitsize of
> +  // m_mode should be exactly the same as the definition of rtx_def,
> +  // aka RTX_MACHINE_MODE_BITSIZE in rtl.h.
> +  machine_mode m_mode : 16;
>  };
>  
>  // A contiguous array of access_info pointers.  Used to represent a
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index f634cab730b..a18ecf7632f 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -63,6 +63,9 @@ enum rtx_code  {
>  # define NON_GENERATOR_NUM_RTX_CODE ((int) MATCH_OPERAND)
>  #endif
>  
> +#define RTX_CODE_BITSIZE 8
> +#define RTX_MACHINE_MODE_BITSIZE 16
> +
>  /* Register Transfer Language EXPRESSIONS CODE CLASSES */
>  
>  enum rtx_class  {
> @@ -310,10 +313,10 @@ struct GTY((desc("0"), tag("0"),
>  	    chain_next ("RTX_NEXT (&%h)"),
>  	    chain_prev ("RTX_PREV (&%h)"))) rtx_def {
>    /* The kind of expression this is.  */
> -  ENUM_BITFIELD(rtx_code) code: 16;
> +  ENUM_BITFIELD(rtx_code) code: RTX_CODE_BITSIZE;
>  
>    /* The kind of value the expression has.  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;

Please swap the fields around, as discussed previously.

>  
>    /* 1 in a MEM if we should keep the alias set for this mem unchanged
>       when we access a component.
> @@ -2164,7 +2167,7 @@ subreg_shape::operator != (const subreg_shape &other) const
>  inline unsigned HOST_WIDE_INT
>  subreg_shape::unique_id () const
>  {
> -  { STATIC_ASSERT (MAX_MACHINE_MODE <= 256); }
> +  { STATIC_ASSERT (MAX_MACHINE_MODE <= (1 << RTX_MACHINE_MODE_BITSIZE)); }
>    { STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 3); }
>    { STATIC_ASSERT (sizeof (offset.coeffs[0]) <= 2); }
>    int res = (int) inner_mode + ((int) outer_mode << 8);
> diff --git a/gcc/rtlanal.h b/gcc/rtlanal.h
> index 5fbed816e20..15aba0dec7a 100644
> --- a/gcc/rtlanal.h
> +++ b/gcc/rtlanal.h
> @@ -99,8 +99,9 @@ public:
>    unsigned int flags : 16;
>  
>    /* The mode of the reference.  If IS_MULTIREG, this is the mode of
> -     REGNO - MULTIREG_OFFSET.  */
> -  machine_mode mode : 8;
> +     REGNO - MULTIREG_OFFSET.  Note the bitsize of mode should be exactly
> +     the same as the definition of rtx_def,  */
> +  machine_mode mode : 16;
>  
>    /* If IS_MULTIREG, the offset of REGNO from the start of the register.  */
>    unsigned int multireg_offset : 8;

Like with ira-int.h, any reasonable use of rtlanal.h will also need rtl.h,
so we should be able to use the BITSIZE macro directly.  Please swap
#includes around if necessary.

> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index a1aea136e75..001d232f433 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1680,8 +1680,11 @@ struct GTY(()) tree_type_common {
>    tree attributes;
>    unsigned int uid;
>  
> +  /* Note the bitsize of wmode should be exactly the same as the
> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */
> +  ENUM_BITFIELD(machine_mode) mode : 16;
> +
>    unsigned int precision : 16;
> -  ENUM_BITFIELD(machine_mode) mode : 8;
>    unsigned lang_flag_0 : 1;
>    unsigned lang_flag_1 : 1;
>    unsigned lang_flag_2 : 1;
> @@ -1712,7 +1715,7 @@ struct GTY(()) tree_type_common {
>    unsigned empty_flag : 1;
>    unsigned indivisible_p : 1;
>    unsigned no_named_args_stdarg_p : 1;
> -  unsigned spare : 9;
> +  unsigned spare : 1;
>  
>    alias_set_type alias_set;
>    tree pointer_to;
> @@ -1770,7 +1773,9 @@ struct GTY(()) tree_decl_common {
>    struct tree_decl_minimal common;
>    tree size;
>  
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  /* Note the bitsize of wmode should be exactly the same as the
> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */
> +  ENUM_BITFIELD(machine_mode) mode : 16;
>  
>    unsigned nonlocal_flag : 1;
>    unsigned virtual_flag : 1;

Please also update:

  /* 13 bits unused.  */

to account for the difference.

Thanks,
Richard

Li, Pan2 via Gcc-patches May 12, 2023, 11:16 a.m. UTC | #3

Thanks Richard for comments. In previous, I am not sure it is reasonable to let everywhere consume the same macro in rtl.h (As the includes you mentioned). Thus, make a conservative change in PATCH v1.

I will address the comments and try to align the bit size to the one and the only one macro soon.

Pan


-----Original Message-----
From: Richard Sandiford <richard.sandiford@arm.com> 
Sent: Friday, May 12, 2023 4:24 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com; rguenther@suse.de
Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits

pan2.li@intel.com writes:
> From: Pan Li <pan2.li@intel.com>
>
> We are running out of the machine_mode(8 bits) in RISC-V backend. Thus 
> we would like to extend the machine mode bit size from 8 to 16 bits.
> However, it is sensitive to extend the memory size in common structure 
> like tree or rtx. This patch would like to extend the machine mode 
> bits to 16 bits by shrinking, like:
>
> * Swap the bit size of code and machine code in rtx_def.
> * Reconcile the machine_mode location and spare in tree.
>
> The memory impact of this patch for correlated structure looks like below:
>
> +-------------------+----------+---------+------+
> | struct/bytes      | upstream | patched | diff |
> +-------------------+----------+---------+------+
> | rtx_obj_reference |        8 |      12 |   +4 |
> | ext_modified      |        2 |       3 |   +1 |
> | ira_allocno       |      192 |     200 |   +8 |
> | qty_table_elem    |       40 |      40 |    0 |
> | reg_stat_type     |       64 |      64 |    0 |
> | rtx_def           |       40 |      40 |    0 |
> | table_elt         |       80 |      80 |    0 |
> | tree_decl_common  |      112 |     112 |    0 |
> | tree_type_common  |      128 |     128 |    0 |
> +-------------------+----------+---------+------+
>
> The tree and rtx related struct has no memory changes after this 
> patch, and the machine_mode changes to 16 bits already.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> Co-authored-by: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
> Co-authored-by: Kito Cheng <kito.cheng@sifive.com>
>
> gcc/ChangeLog:
>
> 	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
> 	* cse.cc (struct qty_table_elem): Ditto.
> 	(struct table_elt): Ditto.
> 	(struct set): Ditto.
> 	* genopinit.cc (main): Reconciled the machine mode limit.
> 	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
> 	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.
> 	* rtl-ssa/accesses.h: Ditto.
> 	* rtl.h (RTX_CODE_BITSIZE): New macro.
> 	(RTX_MACHINE_MODE_BITSIZE): Ditto.
> 	(struct GTY): Swap bit size between code and machine mode.
> 	(subreg_shape::unique_id): Reconciled the machine mode limit.
> 	* rtlanal.h: Extended machine mode to 16 bits.
> 	* tree-core.h (struct tree_type_common): Ditto.
> 	(struct tree_decl_common): Reconciled the locate and extended
> 	bit size of machine mode.
> ---
>  gcc/combine.cc         |  4 ++--
>  gcc/cse.cc             |  8 ++++----
>  gcc/genopinit.cc       |  3 ++-
>  gcc/ira-int.h          | 12 ++++++++----
>  gcc/ree.cc             |  2 +-
>  gcc/rtl-ssa/accesses.h |  6 ++++--
>  gcc/rtl.h              |  9 ++++++---
>  gcc/rtlanal.h          |  5 +++--
>  gcc/tree-core.h        | 11 ++++++++---
>  9 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/gcc/combine.cc b/gcc/combine.cc index 
> 5aa0ec5c45a..bdf6f635c80 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -200,7 +200,7 @@ struct reg_stat_type {
>  
>    unsigned HOST_WIDE_INT	last_set_nonzero_bits;
>    char				last_set_sign_bit_copies;
> -  ENUM_BITFIELD(machine_mode)	last_set_mode : 8;
> +  ENUM_BITFIELD(machine_mode)	last_set_mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* Set nonzero if references to register n in expressions should not be
>       used.  last_set_invalid is set nonzero when this register is 
> being @@ -235,7 +235,7 @@ struct reg_stat_type {
>       truncation if we know that value already contains a truncated
>       value.  */
>  
> -  ENUM_BITFIELD(machine_mode)	truncated_to_mode : 8;
> +  ENUM_BITFIELD(machine_mode)	truncated_to_mode : RTX_MACHINE_MODE_BITSIZE;
>  };
>  
>  
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index b10c9b0c94d..fe594c1bc3d 100644
> --- a/gcc/cse.cc
> +++ b/gcc/cse.cc
> @@ -250,8 +250,8 @@ struct qty_table_elem
>    unsigned int first_reg, last_reg;
>    /* The sizes of these fields should match the sizes of the
>       code and mode fields of struct rtx_def (see rtl.h).  */

The comment can be removed, since you're now adding macros to ensure this (thanks).  Same for other instances of the comment.

> -  ENUM_BITFIELD(rtx_code) comparison_code : 16;
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(rtx_code) comparison_code : RTX_CODE_BITSIZE;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;

Please put the mode first, so that the 16-bit value is aligned to 16 bits.

>  };
>  
>  /* The table of all qtys, indexed by qty number.  */ @@ -406,7 +406,7 
> @@ struct table_elt
>    int regcost;
>    /* The size of this field should match the size
>       of the mode field of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>    char in_memory;
>    char is_const;
>    char flag;
> @@ -4155,7 +4155,7 @@ struct set
>    /* Original machine mode, in case it becomes a CONST_INT.
>       The size of this field should match the size of the mode
>       field of struct rtx_def (see rtl.h).  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>    /* Hash value of constant equivalent for SET_SRC.  */
>    unsigned src_const_hash;
>    /* A constant equivalent for SET_SRC, if any.  */ diff --git 
> a/gcc/genopinit.cc b/gcc/genopinit.cc index 83cb7504fa1..2add8b925da 
> 100644
> --- a/gcc/genopinit.cc
> +++ b/gcc/genopinit.cc
> @@ -182,7 +182,8 @@ main (int argc, const char **argv)
>  
>    progname = "genopinit";
>  
> -  if (NUM_OPTABS > 0xffff || MAX_MACHINE_MODE >= 0xff)
> +  if (NUM_OPTABS > 0xffff
> +    || MAX_MACHINE_MODE >= ((1 << RTX_MACHINE_MODE_BITSIZE) - 1))
>      fatal ("genopinit range assumptions invalid");
>  
>    if (!init_rtx_reader_args_cb (argc, argv, handle_arg)) diff --git 
> a/gcc/ira-int.h b/gcc/ira-int.h index e2de47213b4..124e14200b1 100644
> --- a/gcc/ira-int.h
> +++ b/gcc/ira-int.h
> @@ -280,11 +280,15 @@ struct ira_allocno
>    /* Regno for allocno or cap.  */
>    int regno;
>    /* Mode of the allocno which is the mode of the corresponding
> -     pseudo-register.  */
> -  ENUM_BITFIELD (machine_mode) mode : 8;
> +     pseudo-register.  Note the bitsize of mode should be exactly
> +     the same as the definition of rtx_def, aka RTX_MACHINE_MODE_BITSIZE
> +     in rtl.h.  */
> +  ENUM_BITFIELD (machine_mode) mode : 16;
>    /* Widest mode of the allocno which in at least one case could be
> -     for paradoxical subregs where wmode > mode.  */
> -  ENUM_BITFIELD (machine_mode) wmode : 8;
> +     for paradoxical subregs where wmode > mode.  Note the bitsize of
> +     wmode should be exactly the same as the definition of rtx_def,
> +     aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */  ENUM_BITFIELD 
> + (machine_mode) wmode : 16;
>    /* Register class which should be used for allocation for given
>       allocno.  NO_REGS means that we should use memory.  */
>    ENUM_BITFIELD (reg_class) aclass : 16;

Why not use the BITSIZE macros directly?  Any reasonable use of ira-int.h will also need rtl.h.  If something includes ira-int.h before rtl.h then we should change that.

To avoid growing this structure, please move something into one of the holes later in the structure.  E.g. hard_regno could go before num_objects.

> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index 413aec7c8eb..fb011bc907c 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -567,7 +567,7 @@ enum ext_modified_kind  struct ATTRIBUTE_PACKED 
> ext_modified  {
>    /* Mode from which ree has zero or sign extended the destination.  
> */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>  
>    /* Kind of modification of the insn.  */
>    ENUM_BITFIELD(ext_modified_kind) kind : 2; diff --git 
> a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h index 
> c5180b9308a..2e73004cafa 100644
> --- a/gcc/rtl-ssa/accesses.h
> +++ b/gcc/rtl-ssa/accesses.h
> @@ -253,8 +253,10 @@ private:
>    // Bits for future expansion.
>    unsigned int m_spare : 2;
>  
> -  // The value returned by the accessor above.
> -  machine_mode m_mode : 8;
> +  // The value returned by the accessor above.  Note the bitsize of  
> + // m_mode should be exactly the same as the definition of rtx_def,  
> + // aka RTX_MACHINE_MODE_BITSIZE in rtl.h.
> +  machine_mode m_mode : 16;
>  };
>  
>  // A contiguous array of access_info pointers.  Used to represent a 
> diff --git a/gcc/rtl.h b/gcc/rtl.h index f634cab730b..a18ecf7632f 
> 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -63,6 +63,9 @@ enum rtx_code  {
>  # define NON_GENERATOR_NUM_RTX_CODE ((int) MATCH_OPERAND)  #endif
>  
> +#define RTX_CODE_BITSIZE 8
> +#define RTX_MACHINE_MODE_BITSIZE 16
> +
>  /* Register Transfer Language EXPRESSIONS CODE CLASSES */
>  
>  enum rtx_class  {
> @@ -310,10 +313,10 @@ struct GTY((desc("0"), tag("0"),
>  	    chain_next ("RTX_NEXT (&%h)"),
>  	    chain_prev ("RTX_PREV (&%h)"))) rtx_def {
>    /* The kind of expression this is.  */
> -  ENUM_BITFIELD(rtx_code) code: 16;
> +  ENUM_BITFIELD(rtx_code) code: RTX_CODE_BITSIZE;
>  
>    /* The kind of value the expression has.  */
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;

Please swap the fields around, as discussed previously.

>  
>    /* 1 in a MEM if we should keep the alias set for this mem unchanged
>       when we access a component.
> @@ -2164,7 +2167,7 @@ subreg_shape::operator != (const subreg_shape 
> &other) const  inline unsigned HOST_WIDE_INT  subreg_shape::unique_id 
> () const  {
> -  { STATIC_ASSERT (MAX_MACHINE_MODE <= 256); }
> +  { STATIC_ASSERT (MAX_MACHINE_MODE <= (1 << 
> + RTX_MACHINE_MODE_BITSIZE)); }
>    { STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 3); }
>    { STATIC_ASSERT (sizeof (offset.coeffs[0]) <= 2); }
>    int res = (int) inner_mode + ((int) outer_mode << 8); diff --git 
> a/gcc/rtlanal.h b/gcc/rtlanal.h index 5fbed816e20..15aba0dec7a 100644
> --- a/gcc/rtlanal.h
> +++ b/gcc/rtlanal.h
> @@ -99,8 +99,9 @@ public:
>    unsigned int flags : 16;
>  
>    /* The mode of the reference.  If IS_MULTIREG, this is the mode of
> -     REGNO - MULTIREG_OFFSET.  */
> -  machine_mode mode : 8;
> +     REGNO - MULTIREG_OFFSET.  Note the bitsize of mode should be exactly
> +     the same as the definition of rtx_def,  */  machine_mode mode : 
> + 16;
>  
>    /* If IS_MULTIREG, the offset of REGNO from the start of the register.  */
>    unsigned int multireg_offset : 8;

Like with ira-int.h, any reasonable use of rtlanal.h will also need rtl.h, so we should be able to use the BITSIZE macro directly.  Please swap #includes around if necessary.

> diff --git a/gcc/tree-core.h b/gcc/tree-core.h index 
> a1aea136e75..001d232f433 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1680,8 +1680,11 @@ struct GTY(()) tree_type_common {
>    tree attributes;
>    unsigned int uid;
>  
> +  /* Note the bitsize of wmode should be exactly the same as the
> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  
> + */
> +  ENUM_BITFIELD(machine_mode) mode : 16;
> +
>    unsigned int precision : 16;
> -  ENUM_BITFIELD(machine_mode) mode : 8;
>    unsigned lang_flag_0 : 1;
>    unsigned lang_flag_1 : 1;
>    unsigned lang_flag_2 : 1;
> @@ -1712,7 +1715,7 @@ struct GTY(()) tree_type_common {
>    unsigned empty_flag : 1;
>    unsigned indivisible_p : 1;
>    unsigned no_named_args_stdarg_p : 1;
> -  unsigned spare : 9;
> +  unsigned spare : 1;
>  
>    alias_set_type alias_set;
>    tree pointer_to;
> @@ -1770,7 +1773,9 @@ struct GTY(()) tree_decl_common {
>    struct tree_decl_minimal common;
>    tree size;
>  
> -  ENUM_BITFIELD(machine_mode) mode : 8;
> +  /* Note the bitsize of wmode should be exactly the same as the
> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  
> + */
> +  ENUM_BITFIELD(machine_mode) mode : 16;
>  
>    unsigned nonlocal_flag : 1;
>    unsigned virtual_flag : 1;

Please also update:

  /* 13 bits unused.  */

to account for the difference.

Thanks,
Richard

Richard Sandiford May 12, 2023, 11:31 a.m. UTC | #4

"Li, Pan2 via Gcc-patches" <gcc-patches@gcc.gnu.org> writes:
> Thanks Richard for comments. In previous, I am not sure it is reasonable to let everywhere consume the same macro in rtl.h (As the includes you mentioned). Thus, make a conservative change in PATCH v1.
>
> I will address the comments and try to align the bit size to the one and the only one macro soon.

Sorry, I should have thought about this earlier, but it would
probably make sense to name the macro MACHINE_MODE_BITSIZE and
define it in machmode.h rather than rtl.h.  (The rtx_code stuff
should stay as-is.)

Thanks,
Richard

>
> Pan
>
>
> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com> 
> Sent: Friday, May 12, 2023 4:24 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com; rguenther@suse.de
> Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits
>
> pan2.li@intel.com writes:
>> From: Pan Li <pan2.li@intel.com>
>>
>> We are running out of the machine_mode(8 bits) in RISC-V backend. Thus 
>> we would like to extend the machine mode bit size from 8 to 16 bits.
>> However, it is sensitive to extend the memory size in common structure 
>> like tree or rtx. This patch would like to extend the machine mode 
>> bits to 16 bits by shrinking, like:
>>
>> * Swap the bit size of code and machine code in rtx_def.
>> * Reconcile the machine_mode location and spare in tree.
>>
>> The memory impact of this patch for correlated structure looks like below:
>>
>> +-------------------+----------+---------+------+
>> | struct/bytes      | upstream | patched | diff |
>> +-------------------+----------+---------+------+
>> | rtx_obj_reference |        8 |      12 |   +4 |
>> | ext_modified      |        2 |       3 |   +1 |
>> | ira_allocno       |      192 |     200 |   +8 |
>> | qty_table_elem    |       40 |      40 |    0 |
>> | reg_stat_type     |       64 |      64 |    0 |
>> | rtx_def           |       40 |      40 |    0 |
>> | table_elt         |       80 |      80 |    0 |
>> | tree_decl_common  |      112 |     112 |    0 |
>> | tree_type_common  |      128 |     128 |    0 |
>> +-------------------+----------+---------+------+
>>
>> The tree and rtx related struct has no memory changes after this 
>> patch, and the machine_mode changes to 16 bits already.
>>
>> Signed-off-by: Pan Li <pan2.li@intel.com>
>> Co-authored-by: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
>> Co-authored-by: Kito Cheng <kito.cheng@sifive.com>
>>
>> gcc/ChangeLog:
>>
>> 	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
>> 	* cse.cc (struct qty_table_elem): Ditto.
>> 	(struct table_elt): Ditto.
>> 	(struct set): Ditto.
>> 	* genopinit.cc (main): Reconciled the machine mode limit.
>> 	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
>> 	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.
>> 	* rtl-ssa/accesses.h: Ditto.
>> 	* rtl.h (RTX_CODE_BITSIZE): New macro.
>> 	(RTX_MACHINE_MODE_BITSIZE): Ditto.
>> 	(struct GTY): Swap bit size between code and machine mode.
>> 	(subreg_shape::unique_id): Reconciled the machine mode limit.
>> 	* rtlanal.h: Extended machine mode to 16 bits.
>> 	* tree-core.h (struct tree_type_common): Ditto.
>> 	(struct tree_decl_common): Reconciled the locate and extended
>> 	bit size of machine mode.
>> ---
>>  gcc/combine.cc         |  4 ++--
>>  gcc/cse.cc             |  8 ++++----
>>  gcc/genopinit.cc       |  3 ++-
>>  gcc/ira-int.h          | 12 ++++++++----
>>  gcc/ree.cc             |  2 +-
>>  gcc/rtl-ssa/accesses.h |  6 ++++--
>>  gcc/rtl.h              |  9 ++++++---
>>  gcc/rtlanal.h          |  5 +++--
>>  gcc/tree-core.h        | 11 ++++++++---
>>  9 files changed, 38 insertions(+), 22 deletions(-)
>>
>> diff --git a/gcc/combine.cc b/gcc/combine.cc index 
>> 5aa0ec5c45a..bdf6f635c80 100644
>> --- a/gcc/combine.cc
>> +++ b/gcc/combine.cc
>> @@ -200,7 +200,7 @@ struct reg_stat_type {
>>  
>>    unsigned HOST_WIDE_INT	last_set_nonzero_bits;
>>    char				last_set_sign_bit_copies;
>> -  ENUM_BITFIELD(machine_mode)	last_set_mode : 8;
>> +  ENUM_BITFIELD(machine_mode)	last_set_mode : RTX_MACHINE_MODE_BITSIZE;
>>  
>>    /* Set nonzero if references to register n in expressions should not be
>>       used.  last_set_invalid is set nonzero when this register is 
>> being @@ -235,7 +235,7 @@ struct reg_stat_type {
>>       truncation if we know that value already contains a truncated
>>       value.  */
>>  
>> -  ENUM_BITFIELD(machine_mode)	truncated_to_mode : 8;
>> +  ENUM_BITFIELD(machine_mode)	truncated_to_mode : RTX_MACHINE_MODE_BITSIZE;
>>  };
>>  
>>  
>> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> index b10c9b0c94d..fe594c1bc3d 100644
>> --- a/gcc/cse.cc
>> +++ b/gcc/cse.cc
>> @@ -250,8 +250,8 @@ struct qty_table_elem
>>    unsigned int first_reg, last_reg;
>>    /* The sizes of these fields should match the sizes of the
>>       code and mode fields of struct rtx_def (see rtl.h).  */
>
> The comment can be removed, since you're now adding macros to ensure this (thanks).  Same for other instances of the comment.
>
>> -  ENUM_BITFIELD(rtx_code) comparison_code : 16;
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(rtx_code) comparison_code : RTX_CODE_BITSIZE;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>
> Please put the mode first, so that the 16-bit value is aligned to 16 bits.
>
>>  };
>>  
>>  /* The table of all qtys, indexed by qty number.  */ @@ -406,7 +406,7 
>> @@ struct table_elt
>>    int regcost;
>>    /* The size of this field should match the size
>>       of the mode field of struct rtx_def (see rtl.h).  */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>>    char in_memory;
>>    char is_const;
>>    char flag;
>> @@ -4155,7 +4155,7 @@ struct set
>>    /* Original machine mode, in case it becomes a CONST_INT.
>>       The size of this field should match the size of the mode
>>       field of struct rtx_def (see rtl.h).  */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>>    /* Hash value of constant equivalent for SET_SRC.  */
>>    unsigned src_const_hash;
>>    /* A constant equivalent for SET_SRC, if any.  */ diff --git 
>> a/gcc/genopinit.cc b/gcc/genopinit.cc index 83cb7504fa1..2add8b925da 
>> 100644
>> --- a/gcc/genopinit.cc
>> +++ b/gcc/genopinit.cc
>> @@ -182,7 +182,8 @@ main (int argc, const char **argv)
>>  
>>    progname = "genopinit";
>>  
>> -  if (NUM_OPTABS > 0xffff || MAX_MACHINE_MODE >= 0xff)
>> +  if (NUM_OPTABS > 0xffff
>> +    || MAX_MACHINE_MODE >= ((1 << RTX_MACHINE_MODE_BITSIZE) - 1))
>>      fatal ("genopinit range assumptions invalid");
>>  
>>    if (!init_rtx_reader_args_cb (argc, argv, handle_arg)) diff --git 
>> a/gcc/ira-int.h b/gcc/ira-int.h index e2de47213b4..124e14200b1 100644
>> --- a/gcc/ira-int.h
>> +++ b/gcc/ira-int.h
>> @@ -280,11 +280,15 @@ struct ira_allocno
>>    /* Regno for allocno or cap.  */
>>    int regno;
>>    /* Mode of the allocno which is the mode of the corresponding
>> -     pseudo-register.  */
>> -  ENUM_BITFIELD (machine_mode) mode : 8;
>> +     pseudo-register.  Note the bitsize of mode should be exactly
>> +     the same as the definition of rtx_def, aka RTX_MACHINE_MODE_BITSIZE
>> +     in rtl.h.  */
>> +  ENUM_BITFIELD (machine_mode) mode : 16;
>>    /* Widest mode of the allocno which in at least one case could be
>> -     for paradoxical subregs where wmode > mode.  */
>> -  ENUM_BITFIELD (machine_mode) wmode : 8;
>> +     for paradoxical subregs where wmode > mode.  Note the bitsize of
>> +     wmode should be exactly the same as the definition of rtx_def,
>> +     aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */  ENUM_BITFIELD 
>> + (machine_mode) wmode : 16;
>>    /* Register class which should be used for allocation for given
>>       allocno.  NO_REGS means that we should use memory.  */
>>    ENUM_BITFIELD (reg_class) aclass : 16;
>
> Why not use the BITSIZE macros directly?  Any reasonable use of ira-int.h will also need rtl.h.  If something includes ira-int.h before rtl.h then we should change that.
>
> To avoid growing this structure, please move something into one of the holes later in the structure.  E.g. hard_regno could go before num_objects.
>
>> diff --git a/gcc/ree.cc b/gcc/ree.cc
>> index 413aec7c8eb..fb011bc907c 100644
>> --- a/gcc/ree.cc
>> +++ b/gcc/ree.cc
>> @@ -567,7 +567,7 @@ enum ext_modified_kind  struct ATTRIBUTE_PACKED 
>> ext_modified  {
>>    /* Mode from which ree has zero or sign extended the destination.  
>> */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>>  
>>    /* Kind of modification of the insn.  */
>>    ENUM_BITFIELD(ext_modified_kind) kind : 2; diff --git 
>> a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h index 
>> c5180b9308a..2e73004cafa 100644
>> --- a/gcc/rtl-ssa/accesses.h
>> +++ b/gcc/rtl-ssa/accesses.h
>> @@ -253,8 +253,10 @@ private:
>>    // Bits for future expansion.
>>    unsigned int m_spare : 2;
>>  
>> -  // The value returned by the accessor above.
>> -  machine_mode m_mode : 8;
>> +  // The value returned by the accessor above.  Note the bitsize of  
>> + // m_mode should be exactly the same as the definition of rtx_def,  
>> + // aka RTX_MACHINE_MODE_BITSIZE in rtl.h.
>> +  machine_mode m_mode : 16;
>>  };
>>  
>>  // A contiguous array of access_info pointers.  Used to represent a 
>> diff --git a/gcc/rtl.h b/gcc/rtl.h index f634cab730b..a18ecf7632f 
>> 100644
>> --- a/gcc/rtl.h
>> +++ b/gcc/rtl.h
>> @@ -63,6 +63,9 @@ enum rtx_code  {
>>  # define NON_GENERATOR_NUM_RTX_CODE ((int) MATCH_OPERAND)  #endif
>>  
>> +#define RTX_CODE_BITSIZE 8
>> +#define RTX_MACHINE_MODE_BITSIZE 16
>> +
>>  /* Register Transfer Language EXPRESSIONS CODE CLASSES */
>>  
>>  enum rtx_class  {
>> @@ -310,10 +313,10 @@ struct GTY((desc("0"), tag("0"),
>>  	    chain_next ("RTX_NEXT (&%h)"),
>>  	    chain_prev ("RTX_PREV (&%h)"))) rtx_def {
>>    /* The kind of expression this is.  */
>> -  ENUM_BITFIELD(rtx_code) code: 16;
>> +  ENUM_BITFIELD(rtx_code) code: RTX_CODE_BITSIZE;
>>  
>>    /* The kind of value the expression has.  */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>
> Please swap the fields around, as discussed previously.
>
>>  
>>    /* 1 in a MEM if we should keep the alias set for this mem unchanged
>>       when we access a component.
>> @@ -2164,7 +2167,7 @@ subreg_shape::operator != (const subreg_shape 
>> &other) const  inline unsigned HOST_WIDE_INT  subreg_shape::unique_id 
>> () const  {
>> -  { STATIC_ASSERT (MAX_MACHINE_MODE <= 256); }
>> +  { STATIC_ASSERT (MAX_MACHINE_MODE <= (1 << 
>> + RTX_MACHINE_MODE_BITSIZE)); }
>>    { STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 3); }
>>    { STATIC_ASSERT (sizeof (offset.coeffs[0]) <= 2); }
>>    int res = (int) inner_mode + ((int) outer_mode << 8); diff --git 
>> a/gcc/rtlanal.h b/gcc/rtlanal.h index 5fbed816e20..15aba0dec7a 100644
>> --- a/gcc/rtlanal.h
>> +++ b/gcc/rtlanal.h
>> @@ -99,8 +99,9 @@ public:
>>    unsigned int flags : 16;
>>  
>>    /* The mode of the reference.  If IS_MULTIREG, this is the mode of
>> -     REGNO - MULTIREG_OFFSET.  */
>> -  machine_mode mode : 8;
>> +     REGNO - MULTIREG_OFFSET.  Note the bitsize of mode should be exactly
>> +     the same as the definition of rtx_def,  */  machine_mode mode : 
>> + 16;
>>  
>>    /* If IS_MULTIREG, the offset of REGNO from the start of the register.  */
>>    unsigned int multireg_offset : 8;
>
> Like with ira-int.h, any reasonable use of rtlanal.h will also need rtl.h, so we should be able to use the BITSIZE macro directly.  Please swap #includes around if necessary.
>
>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h index 
>> a1aea136e75..001d232f433 100644
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -1680,8 +1680,11 @@ struct GTY(()) tree_type_common {
>>    tree attributes;
>>    unsigned int uid;
>>  
>> +  /* Note the bitsize of wmode should be exactly the same as the
>> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  
>> + */
>> +  ENUM_BITFIELD(machine_mode) mode : 16;
>> +
>>    unsigned int precision : 16;
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>>    unsigned lang_flag_0 : 1;
>>    unsigned lang_flag_1 : 1;
>>    unsigned lang_flag_2 : 1;
>> @@ -1712,7 +1715,7 @@ struct GTY(()) tree_type_common {
>>    unsigned empty_flag : 1;
>>    unsigned indivisible_p : 1;
>>    unsigned no_named_args_stdarg_p : 1;
>> -  unsigned spare : 9;
>> +  unsigned spare : 1;
>>  
>>    alias_set_type alias_set;
>>    tree pointer_to;
>> @@ -1770,7 +1773,9 @@ struct GTY(()) tree_decl_common {
>>    struct tree_decl_minimal common;
>>    tree size;
>>  
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  /* Note the bitsize of wmode should be exactly the same as the
>> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  
>> + */
>> +  ENUM_BITFIELD(machine_mode) mode : 16;
>>  
>>    unsigned nonlocal_flag : 1;
>>    unsigned virtual_flag : 1;
>
> Please also update:
>
>   /* 13 bits unused.  */
>
> to account for the difference.
>
> Thanks,
> Richard

Li, Pan2 via Gcc-patches May 12, 2023, 11:48 a.m. UTC | #5

Never minder. When preparing the PR, I am keeping ask myself that is everywhere about machine code bit size updated? Thus would like to align the bit size to one macro, to avoid developers (perhaps myself in future) suffering such kind of concern.

Will try to move the machine mode to machmode.h.

Pan

-----Original Message-----
From: Richard Sandiford <richard.sandiford@arm.com> 
Sent: Friday, May 12, 2023 7:32 PM
To: Li, Pan2 via Gcc-patches <gcc-patches@gcc.gnu.org>
Cc: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com; rguenther@suse.de
Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits

"Li, Pan2 via Gcc-patches" <gcc-patches@gcc.gnu.org> writes:
> Thanks Richard for comments. In previous, I am not sure it is reasonable to let everywhere consume the same macro in rtl.h (As the includes you mentioned). Thus, make a conservative change in PATCH v1.
>
> I will address the comments and try to align the bit size to the one and the only one macro soon.

Sorry, I should have thought about this earlier, but it would probably make sense to name the macro MACHINE_MODE_BITSIZE and define it in machmode.h rather than rtl.h.  (The rtx_code stuff should stay as-is.)

Thanks,
Richard

>
> Pan
>
>
> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: Friday, May 12, 2023 4:24 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; 
> kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; 
> jeffreyalaw@gmail.com; rguenther@suse.de
> Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 
> bits
>
> pan2.li@intel.com writes:
>> From: Pan Li <pan2.li@intel.com>
>>
>> We are running out of the machine_mode(8 bits) in RISC-V backend. 
>> Thus we would like to extend the machine mode bit size from 8 to 16 bits.
>> However, it is sensitive to extend the memory size in common 
>> structure like tree or rtx. This patch would like to extend the 
>> machine mode bits to 16 bits by shrinking, like:
>>
>> * Swap the bit size of code and machine code in rtx_def.
>> * Reconcile the machine_mode location and spare in tree.
>>
>> The memory impact of this patch for correlated structure looks like below:
>>
>> +-------------------+----------+---------+------+
>> | struct/bytes      | upstream | patched | diff |
>> +-------------------+----------+---------+------+
>> | rtx_obj_reference |        8 |      12 |   +4 |
>> | ext_modified      |        2 |       3 |   +1 |
>> | ira_allocno       |      192 |     200 |   +8 |
>> | qty_table_elem    |       40 |      40 |    0 |
>> | reg_stat_type     |       64 |      64 |    0 |
>> | rtx_def           |       40 |      40 |    0 |
>> | table_elt         |       80 |      80 |    0 |
>> | tree_decl_common  |      112 |     112 |    0 |
>> | tree_type_common  |      128 |     128 |    0 |
>> +-------------------+----------+---------+------+
>>
>> The tree and rtx related struct has no memory changes after this 
>> patch, and the machine_mode changes to 16 bits already.
>>
>> Signed-off-by: Pan Li <pan2.li@intel.com>
>> Co-authored-by: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
>> Co-authored-by: Kito Cheng <kito.cheng@sifive.com>
>>
>> gcc/ChangeLog:
>>
>> 	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
>> 	* cse.cc (struct qty_table_elem): Ditto.
>> 	(struct table_elt): Ditto.
>> 	(struct set): Ditto.
>> 	* genopinit.cc (main): Reconciled the machine mode limit.
>> 	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
>> 	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.
>> 	* rtl-ssa/accesses.h: Ditto.
>> 	* rtl.h (RTX_CODE_BITSIZE): New macro.
>> 	(RTX_MACHINE_MODE_BITSIZE): Ditto.
>> 	(struct GTY): Swap bit size between code and machine mode.
>> 	(subreg_shape::unique_id): Reconciled the machine mode limit.
>> 	* rtlanal.h: Extended machine mode to 16 bits.
>> 	* tree-core.h (struct tree_type_common): Ditto.
>> 	(struct tree_decl_common): Reconciled the locate and extended
>> 	bit size of machine mode.
>> ---
>>  gcc/combine.cc         |  4 ++--
>>  gcc/cse.cc             |  8 ++++----
>>  gcc/genopinit.cc       |  3 ++-
>>  gcc/ira-int.h          | 12 ++++++++----
>>  gcc/ree.cc             |  2 +-
>>  gcc/rtl-ssa/accesses.h |  6 ++++--
>>  gcc/rtl.h              |  9 ++++++---
>>  gcc/rtlanal.h          |  5 +++--
>>  gcc/tree-core.h        | 11 ++++++++---
>>  9 files changed, 38 insertions(+), 22 deletions(-)
>>
>> diff --git a/gcc/combine.cc b/gcc/combine.cc index
>> 5aa0ec5c45a..bdf6f635c80 100644
>> --- a/gcc/combine.cc
>> +++ b/gcc/combine.cc
>> @@ -200,7 +200,7 @@ struct reg_stat_type {
>>  
>>    unsigned HOST_WIDE_INT	last_set_nonzero_bits;
>>    char				last_set_sign_bit_copies;
>> -  ENUM_BITFIELD(machine_mode)	last_set_mode : 8;
>> +  ENUM_BITFIELD(machine_mode)	last_set_mode : RTX_MACHINE_MODE_BITSIZE;
>>  
>>    /* Set nonzero if references to register n in expressions should not be
>>       used.  last_set_invalid is set nonzero when this register is 
>> being @@ -235,7 +235,7 @@ struct reg_stat_type {
>>       truncation if we know that value already contains a truncated
>>       value.  */
>>  
>> -  ENUM_BITFIELD(machine_mode)	truncated_to_mode : 8;
>> +  ENUM_BITFIELD(machine_mode)	truncated_to_mode : RTX_MACHINE_MODE_BITSIZE;
>>  };
>>  
>>  
>> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> index b10c9b0c94d..fe594c1bc3d 100644
>> --- a/gcc/cse.cc
>> +++ b/gcc/cse.cc
>> @@ -250,8 +250,8 @@ struct qty_table_elem
>>    unsigned int first_reg, last_reg;
>>    /* The sizes of these fields should match the sizes of the
>>       code and mode fields of struct rtx_def (see rtl.h).  */
>
> The comment can be removed, since you're now adding macros to ensure this (thanks).  Same for other instances of the comment.
>
>> -  ENUM_BITFIELD(rtx_code) comparison_code : 16;
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(rtx_code) comparison_code : RTX_CODE_BITSIZE;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>
> Please put the mode first, so that the 16-bit value is aligned to 16 bits.
>
>>  };
>>  
>>  /* The table of all qtys, indexed by qty number.  */ @@ -406,7 
>> +406,7 @@ struct table_elt
>>    int regcost;
>>    /* The size of this field should match the size
>>       of the mode field of struct rtx_def (see rtl.h).  */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>>    char in_memory;
>>    char is_const;
>>    char flag;
>> @@ -4155,7 +4155,7 @@ struct set
>>    /* Original machine mode, in case it becomes a CONST_INT.
>>       The size of this field should match the size of the mode
>>       field of struct rtx_def (see rtl.h).  */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>>    /* Hash value of constant equivalent for SET_SRC.  */
>>    unsigned src_const_hash;
>>    /* A constant equivalent for SET_SRC, if any.  */ diff --git 
>> a/gcc/genopinit.cc b/gcc/genopinit.cc index 83cb7504fa1..2add8b925da
>> 100644
>> --- a/gcc/genopinit.cc
>> +++ b/gcc/genopinit.cc
>> @@ -182,7 +182,8 @@ main (int argc, const char **argv)
>>  
>>    progname = "genopinit";
>>  
>> -  if (NUM_OPTABS > 0xffff || MAX_MACHINE_MODE >= 0xff)
>> +  if (NUM_OPTABS > 0xffff
>> +    || MAX_MACHINE_MODE >= ((1 << RTX_MACHINE_MODE_BITSIZE) - 1))
>>      fatal ("genopinit range assumptions invalid");
>>  
>>    if (!init_rtx_reader_args_cb (argc, argv, handle_arg)) diff --git 
>> a/gcc/ira-int.h b/gcc/ira-int.h index e2de47213b4..124e14200b1 100644
>> --- a/gcc/ira-int.h
>> +++ b/gcc/ira-int.h
>> @@ -280,11 +280,15 @@ struct ira_allocno
>>    /* Regno for allocno or cap.  */
>>    int regno;
>>    /* Mode of the allocno which is the mode of the corresponding
>> -     pseudo-register.  */
>> -  ENUM_BITFIELD (machine_mode) mode : 8;
>> +     pseudo-register.  Note the bitsize of mode should be exactly
>> +     the same as the definition of rtx_def, aka RTX_MACHINE_MODE_BITSIZE
>> +     in rtl.h.  */
>> +  ENUM_BITFIELD (machine_mode) mode : 16;
>>    /* Widest mode of the allocno which in at least one case could be
>> -     for paradoxical subregs where wmode > mode.  */
>> -  ENUM_BITFIELD (machine_mode) wmode : 8;
>> +     for paradoxical subregs where wmode > mode.  Note the bitsize of
>> +     wmode should be exactly the same as the definition of rtx_def,
>> +     aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  */  ENUM_BITFIELD
>> + (machine_mode) wmode : 16;
>>    /* Register class which should be used for allocation for given
>>       allocno.  NO_REGS means that we should use memory.  */
>>    ENUM_BITFIELD (reg_class) aclass : 16;
>
> Why not use the BITSIZE macros directly?  Any reasonable use of ira-int.h will also need rtl.h.  If something includes ira-int.h before rtl.h then we should change that.
>
> To avoid growing this structure, please move something into one of the holes later in the structure.  E.g. hard_regno could go before num_objects.
>
>> diff --git a/gcc/ree.cc b/gcc/ree.cc
>> index 413aec7c8eb..fb011bc907c 100644
>> --- a/gcc/ree.cc
>> +++ b/gcc/ree.cc
>> @@ -567,7 +567,7 @@ enum ext_modified_kind  struct ATTRIBUTE_PACKED 
>> ext_modified  {
>>    /* Mode from which ree has zero or sign extended the destination.  
>> */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>>  
>>    /* Kind of modification of the insn.  */
>>    ENUM_BITFIELD(ext_modified_kind) kind : 2; diff --git 
>> a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h index 
>> c5180b9308a..2e73004cafa 100644
>> --- a/gcc/rtl-ssa/accesses.h
>> +++ b/gcc/rtl-ssa/accesses.h
>> @@ -253,8 +253,10 @@ private:
>>    // Bits for future expansion.
>>    unsigned int m_spare : 2;
>>  
>> -  // The value returned by the accessor above.
>> -  machine_mode m_mode : 8;
>> +  // The value returned by the accessor above.  Note the bitsize of 
>> + // m_mode should be exactly the same as the definition of rtx_def, 
>> + // aka RTX_MACHINE_MODE_BITSIZE in rtl.h.
>> +  machine_mode m_mode : 16;
>>  };
>>  
>>  // A contiguous array of access_info pointers.  Used to represent a 
>> diff --git a/gcc/rtl.h b/gcc/rtl.h index f634cab730b..a18ecf7632f
>> 100644
>> --- a/gcc/rtl.h
>> +++ b/gcc/rtl.h
>> @@ -63,6 +63,9 @@ enum rtx_code  {
>>  # define NON_GENERATOR_NUM_RTX_CODE ((int) MATCH_OPERAND)  #endif
>>  
>> +#define RTX_CODE_BITSIZE 8
>> +#define RTX_MACHINE_MODE_BITSIZE 16
>> +
>>  /* Register Transfer Language EXPRESSIONS CODE CLASSES */
>>  
>>  enum rtx_class  {
>> @@ -310,10 +313,10 @@ struct GTY((desc("0"), tag("0"),
>>  	    chain_next ("RTX_NEXT (&%h)"),
>>  	    chain_prev ("RTX_PREV (&%h)"))) rtx_def {
>>    /* The kind of expression this is.  */
>> -  ENUM_BITFIELD(rtx_code) code: 16;
>> +  ENUM_BITFIELD(rtx_code) code: RTX_CODE_BITSIZE;
>>  
>>    /* The kind of value the expression has.  */
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  ENUM_BITFIELD(machine_mode) mode : RTX_MACHINE_MODE_BITSIZE;
>
> Please swap the fields around, as discussed previously.
>
>>  
>>    /* 1 in a MEM if we should keep the alias set for this mem unchanged
>>       when we access a component.
>> @@ -2164,7 +2167,7 @@ subreg_shape::operator != (const subreg_shape
>> &other) const  inline unsigned HOST_WIDE_INT  subreg_shape::unique_id
>> () const  {
>> -  { STATIC_ASSERT (MAX_MACHINE_MODE <= 256); }
>> +  { STATIC_ASSERT (MAX_MACHINE_MODE <= (1 << 
>> + RTX_MACHINE_MODE_BITSIZE)); }
>>    { STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 3); }
>>    { STATIC_ASSERT (sizeof (offset.coeffs[0]) <= 2); }
>>    int res = (int) inner_mode + ((int) outer_mode << 8); diff --git 
>> a/gcc/rtlanal.h b/gcc/rtlanal.h index 5fbed816e20..15aba0dec7a 100644
>> --- a/gcc/rtlanal.h
>> +++ b/gcc/rtlanal.h
>> @@ -99,8 +99,9 @@ public:
>>    unsigned int flags : 16;
>>  
>>    /* The mode of the reference.  If IS_MULTIREG, this is the mode of
>> -     REGNO - MULTIREG_OFFSET.  */
>> -  machine_mode mode : 8;
>> +     REGNO - MULTIREG_OFFSET.  Note the bitsize of mode should be exactly
>> +     the same as the definition of rtx_def,  */  machine_mode mode : 
>> + 16;
>>  
>>    /* If IS_MULTIREG, the offset of REGNO from the start of the register.  */
>>    unsigned int multireg_offset : 8;
>
> Like with ira-int.h, any reasonable use of rtlanal.h will also need rtl.h, so we should be able to use the BITSIZE macro directly.  Please swap #includes around if necessary.
>
>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h index
>> a1aea136e75..001d232f433 100644
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -1680,8 +1680,11 @@ struct GTY(()) tree_type_common {
>>    tree attributes;
>>    unsigned int uid;
>>  
>> +  /* Note the bitsize of wmode should be exactly the same as the
>> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  
>> + */
>> +  ENUM_BITFIELD(machine_mode) mode : 16;
>> +
>>    unsigned int precision : 16;
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>>    unsigned lang_flag_0 : 1;
>>    unsigned lang_flag_1 : 1;
>>    unsigned lang_flag_2 : 1;
>> @@ -1712,7 +1715,7 @@ struct GTY(()) tree_type_common {
>>    unsigned empty_flag : 1;
>>    unsigned indivisible_p : 1;
>>    unsigned no_named_args_stdarg_p : 1;
>> -  unsigned spare : 9;
>> +  unsigned spare : 1;
>>  
>>    alias_set_type alias_set;
>>    tree pointer_to;
>> @@ -1770,7 +1773,9 @@ struct GTY(()) tree_decl_common {
>>    struct tree_decl_minimal common;
>>    tree size;
>>  
>> -  ENUM_BITFIELD(machine_mode) mode : 8;
>> +  /* Note the bitsize of wmode should be exactly the same as the
>> +     definition of rtx_def,  aka RTX_MACHINE_MODE_BITSIZE in rtl.h.  
>> + */
>> +  ENUM_BITFIELD(machine_mode) mode : 16;
>>  
>>    unsigned nonlocal_flag : 1;
>>    unsigned virtual_flag : 1;
>
> Please also update:
>
>   /* 13 bits unused.  */
>
> to account for the difference.
>
> Thanks,
> Richard

Bernhard Reutner-Fischer May 12, 2023, 7:14 p.m. UTC | #6

On 12 May 2023 08:49:53 CEST, Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

>> gcc/ChangeLog:
>> 
>> 	* combine.cc (struct reg_stat_type): Extended machine mode to 16 bits.
>> 	* cse.cc (struct qty_table_elem): Ditto.
>> 	(struct table_elt): Ditto.
>> 	(struct set): Ditto.
>> 	* genopinit.cc (main): Reconciled the machine mode limit.
>> 	* ira-int.h (struct ira_allocno): Extended machine mode to 16 bits.
>> 	* ree.cc (struct ATTRIBUTE_PACKED): Ditto.
>
>please go over the ChangeLog and properly specify the structure types
>altered.  The script generating the changelog isn't perfect.

And a nitpick, please use present tense in the ChangeLog: Extend, etc
And I wouldn't have said reconcile.

thanks,

Kito Cheng May 13, 2023, 8:44 a.m. UTC | #7

Hi Pan:

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:


diff --git a/gcc/genmodes.cc b/gcc/genmodes.cc
index 715787b8f483..55ac2adb5596 100644
--- 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");

  for_all_modes (c, m)
    tagged_printf ("E_%smode",
@@ -1537,7 +1537,7 @@ emit_mode_wider (void)
                  m->name);

  print_closer ();
-  print_decl ("unsigned char", "mode_wider", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_wider", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    {
@@ -1568,7 +1568,7 @@ emit_mode_wider (void)
    }

  print_closer ();
-  print_decl ("unsigned char", "mode_2xwider", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_2xwider", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    {
@@ -1625,7 +1625,7 @@ emit_mode_complex (void)
  int c;
  struct mode_data *m;

-  print_decl ("unsigned char", "mode_complex", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_complex", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    tagged_printf ("E_%smode",
@@ -1665,7 +1665,7 @@ emit_mode_inner (void)
  int c;
  struct mode_data *m;

-  print_decl ("unsigned char", "mode_inner", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_inner", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    tagged_printf ("E_%smode",
@@ -1738,7 +1738,7 @@ emit_class_narrowest_mode (void)
{
  int c;

-  print_decl ("unsigned char", "class_narrowest_mode", "MAX_MODE_CLASS");
+  print_decl ("unsigned short", "class_narrowest_mode", "MAX_MODE_CLASS");

  for (c = 0; c < MAX_MODE_CLASS; c++)
    {
diff --git a/gcc/machmode.h b/gcc/machmode.h
index f1865c1ef425..a168d6f0da2e 100644
--- a/gcc/machmode.h
+++ b/gcc/machmode.h
@@ -24,13 +24,13 @@ typedef opt_mode<machine_mode> opt_machine_mode;

extern CONST_MODE_SIZE poly_uint16_pod mode_size[NUM_MACHINE_MODES];
extern CONST_MODE_PRECISION poly_uint16_pod mode_precision[NUM_MACHINE_MODES];
-extern const unsigned char mode_inner[NUM_MACHINE_MODES];
+extern const unsigned short mode_inner[NUM_MACHINE_MODES];
extern CONST_MODE_NUNITS poly_uint16_pod mode_nunits[NUM_MACHINE_MODES];
extern CONST_MODE_UNIT_SIZE unsigned char mode_unit_size[NUM_MACHINE_MODES];
extern const unsigned short mode_unit_precision[NUM_MACHINE_MODES];
-extern const unsigned char mode_next[NUM_MACHINE_MODES];
-extern const unsigned char mode_wider[NUM_MACHINE_MODES];
-extern const unsigned char mode_2xwider[NUM_MACHINE_MODES];
+extern const unsigned short mode_next[NUM_MACHINE_MODES];
+extern const unsigned short mode_wider[NUM_MACHINE_MODES];
+extern const unsigned short mode_2xwider[NUM_MACHINE_MODES];

template<typename T>
struct mode_traits
@@ -797,7 +797,7 @@ GET_MODE_2XWIDER_MODE (const T &m)
}

/* Get the complex mode from the component mode.  */
-extern const unsigned char mode_complex[NUM_MACHINE_MODES];
+extern const unsigned short mode_complex[NUM_MACHINE_MODES];
#define GET_MODE_COMPLEX_MODE(MODE) ((machine_mode) mode_complex[MODE])

/* Represents a machine mode that must have a fixed size.  The main
@@ -946,7 +946,7 @@ extern unsigned get_mode_alignment (machine_mode);

/* For each class, get the narrowest mode in that class.  */

-extern const unsigned char class_narrowest_mode[MAX_MODE_CLASS];
+extern const unsigned short class_narrowest_mode[MAX_MODE_CLASS];
#define GET_CLASS_NARROWEST_MODE(CLASS) \
  ((machine_mode) class_narrowest_mode[CLASS])

--
2.39.2

Li, Pan2 via Gcc-patches May 13, 2023, 12:26 p.m. UTC | #8

Oops, looks missed this part when I search all machine_mode by word. Thanks kito for catching this.

It seems there is no easy way to remind the developer to change (for example 16 to 32 bits) the below part in future, how about add some comments to MACHINE_MODE_BITSIZE for this?

Pan

-----Original Message-----
From: Kito Cheng <kito.cheng@sifive.com> 
Sent: Saturday, May 13, 2023 4:45 PM
To: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>
Cc: Richard Biener <rguenther@suse.de>; Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org>; Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com; richard.sandiford@arm.com
Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits

Hi Pan:

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:


diff --git a/gcc/genmodes.cc b/gcc/genmodes.cc index 715787b8f483..55ac2adb5596 100644
--- 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");

  for_all_modes (c, m)
    tagged_printf ("E_%smode",
@@ -1537,7 +1537,7 @@ emit_mode_wider (void)
                  m->name);

  print_closer ();
-  print_decl ("unsigned char", "mode_wider", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_wider", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    {
@@ -1568,7 +1568,7 @@ emit_mode_wider (void)
    }

  print_closer ();
-  print_decl ("unsigned char", "mode_2xwider", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_2xwider", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    {
@@ -1625,7 +1625,7 @@ emit_mode_complex (void)
  int c;
  struct mode_data *m;

-  print_decl ("unsigned char", "mode_complex", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_complex", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    tagged_printf ("E_%smode",
@@ -1665,7 +1665,7 @@ emit_mode_inner (void)
  int c;
  struct mode_data *m;

-  print_decl ("unsigned char", "mode_inner", "NUM_MACHINE_MODES");
+  print_decl ("unsigned short", "mode_inner", "NUM_MACHINE_MODES");

  for_all_modes (c, m)
    tagged_printf ("E_%smode",
@@ -1738,7 +1738,7 @@ emit_class_narrowest_mode (void) {
  int c;

-  print_decl ("unsigned char", "class_narrowest_mode", "MAX_MODE_CLASS");
+  print_decl ("unsigned short", "class_narrowest_mode", 
+ "MAX_MODE_CLASS");

  for (c = 0; c < MAX_MODE_CLASS; c++)
    {
diff --git a/gcc/machmode.h b/gcc/machmode.h index f1865c1ef425..a168d6f0da2e 100644
--- a/gcc/machmode.h
+++ b/gcc/machmode.h
@@ -24,13 +24,13 @@ typedef opt_mode<machine_mode> opt_machine_mode;

extern CONST_MODE_SIZE poly_uint16_pod mode_size[NUM_MACHINE_MODES]; extern CONST_MODE_PRECISION poly_uint16_pod mode_precision[NUM_MACHINE_MODES];
-extern const unsigned char mode_inner[NUM_MACHINE_MODES];
+extern const unsigned short mode_inner[NUM_MACHINE_MODES];
extern CONST_MODE_NUNITS poly_uint16_pod mode_nunits[NUM_MACHINE_MODES]; extern CONST_MODE_UNIT_SIZE unsigned char mode_unit_size[NUM_MACHINE_MODES];
extern const unsigned short mode_unit_precision[NUM_MACHINE_MODES];
-extern const unsigned char mode_next[NUM_MACHINE_MODES]; -extern const unsigned char mode_wider[NUM_MACHINE_MODES]; -extern const unsigned char mode_2xwider[NUM_MACHINE_MODES];
+extern const unsigned short mode_next[NUM_MACHINE_MODES]; extern const 
+unsigned short mode_wider[NUM_MACHINE_MODES]; extern const unsigned 
+short mode_2xwider[NUM_MACHINE_MODES];

template<typename T>
struct mode_traits
@@ -797,7 +797,7 @@ GET_MODE_2XWIDER_MODE (const T &m) }

/* Get the complex mode from the component mode.  */ -extern const unsigned char mode_complex[NUM_MACHINE_MODES];
+extern const unsigned short mode_complex[NUM_MACHINE_MODES];
#define GET_MODE_COMPLEX_MODE(MODE) ((machine_mode) mode_complex[MODE])

/* Represents a machine mode that must have a fixed size.  The main @@ -946,7 +946,7 @@ extern unsigned get_mode_alignment (machine_mode);

/* For each class, get the narrowest mode in that class.  */

-extern const unsigned char class_narrowest_mode[MAX_MODE_CLASS];
+extern const unsigned short class_narrowest_mode[MAX_MODE_CLASS];
#define GET_CLASS_NARROWEST_MODE(CLASS) \
  ((machine_mode) class_narrowest_mode[CLASS])

--
2.39.2

Machine_Mode: Extend machine_mode from 8 to 16 bits

Commit Message

Comments

Patch