Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits

[Richard Sandiford <richard.sandiford@arm.com>]

"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