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.sandif...@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...@intel.com>; 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
"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.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
