From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 069CA3858C54 for ; Fri, 12 May 2023 11:32:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 069CA3858C54 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 46F53FEC; Fri, 12 May 2023 04:32:46 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A77083F67D; Fri, 12 May 2023 04:32:00 -0700 (PDT) From: Richard Sandiford To: "Li\, Pan2 via Gcc-patches" Mail-Followup-To: "Li\, Pan2 via Gcc-patches" ,"Li\, Pan2" , "juzhe.zhong\@rivai.ai" , "kito.cheng\@sifive.com" , "Wang\, Yanzhang" , "jeffreyalaw\@gmail.com" , "rguenther\@suse.de" , richard.sandiford@arm.com Cc: "Li\, Pan2" , "juzhe.zhong\@rivai.ai" , "kito.cheng\@sifive.com" , "Wang\, Yanzhang" , "jeffreyalaw\@gmail.com" , "rguenther\@suse.de" Subject: Re: [PATCH] Machine_Mode: Extend machine_mode from 8 to 16 bits References: <20230512050016.476110-1-pan2.li@intel.com> Date: Fri, 12 May 2023 12:31:59 +0100 In-Reply-To: (Pan Li's message of "Fri, 12 May 2023 11:16:17 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-29.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Li, Pan2 via Gcc-patches" 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 > Sent: Friday, May 12, 2023 4:24 PM > To: Li, Pan2 > Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang ; 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 >> >> 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 >> Co-authored-by: Ju-Zhe Zhong >> Co-authored-by: Kito Cheng >> >> 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