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.sandif...@arm.com>
Sent: Friday, May 12, 2023 4:24 PM
To: Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang,
Yanzhang <yanzhang.w...@intel.com>; jeffreya...@gmail.com; rguent...@suse.de
Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits
pan2...@intel.com writes:
> From: Pan Li <pan2...@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...@intel.com>
> Co-authored-by: Ju-Zhe Zhong <juzhe.zh...@rivai.ai>
> Co-authored-by: Kito Cheng <kito.ch...@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
