Thanks Kito, will commit it after the ICE fix. Pan
-----Original Message----- From: Kito Cheng <kito.ch...@gmail.com> Sent: Thursday, March 21, 2024 8:33 PM To: Li, Pan2 <pan2...@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; Wang, Yanzhang <yanzhang.w...@intel.com> Subject: Re: [PATCH v1] RISC-V: Bugfix function target attribute pollution LGTM, thanks :) On Wed, Mar 20, 2024 at 2:07 PM <pan2...@intel.com> wrote: > > From: Pan Li <pan2...@intel.com> > > This patch depends on below ICE fix. > > https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647915.html > > The function target attribute should be on a per-function basis. > For example, we have 3 function as below: > > void test_1 () {} > > void __attribute__((target("arch=+v"))) test_2 () {} > > void __attribute__((target("arch=+zfh"))) test_3 () {} > > void test_4 () {} > > The scope of the target attribute should not extend the function body. > Aka, test_3 cannot have the 'v' extension, as well as the test_4 > cannot have both the 'v' and 'zfh' extension. > > Unfortunately, for now the test_4 is able to leverage the 'v' and > the 'zfh' extension which is incorrect. This patch would like to > fix the sticking attribute by introduce the commandline subset_list. > When parse_arch, we always clone from the cmdline_subset_list instead > of the current_subset_list. > > Meanwhile, we correct the print information about arch like below. > > .option arch, rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zicsr2p0_zifencei2p0_zbb1p0 > > The riscv_declare_function_name hook is always after the hook > riscv_process_target_attr. Thus, we introduce one hash_map to record > the 1:1 mapping from fndel to its' subset_list in advance. And later > the riscv_declare_function_name is able to get the right information > about the arch. > > Below test are passed for this patch > * The riscv fully regression test. > > PR target/114352 > > gcc/ChangeLog: > > * common/config/riscv/riscv-common.cc (struct riscv_func_target_info): > New struct for func decl and target name. > (struct riscv_func_target_hasher): New hasher for hash table mapping > from the fn_decl to fn_target_name. > (riscv_func_decl_hash): New func to compute the hash for fn_decl. > (riscv_func_target_hasher::hash): New func to impl hash interface. > (riscv_func_target_hasher::equal): New func to impl equal interface. > (riscv_cmdline_subset_list): New static var for cmdline subset list. > (riscv_func_target_table_lazy_init): New func to lazy init the func > target hash table. > (riscv_func_target_get): New func to get target name from hash table. > (riscv_func_target_put): New func to put target name into hash table. > (riscv_func_target_remove_and_destory): New func to remove target > info from the hash table and destory it. > (riscv_parse_arch_string): Set the static var cmdline_subset_list. > * config/riscv/riscv-subset.h (riscv_cmdline_subset_list): New static > var for cmdline subset list. > (riscv_func_target_get): New func decl. > (riscv_func_target_put): Ditto. > (riscv_func_target_remove_and_destory): Ditto. > * config/riscv/riscv-target-attr.cc > (riscv_target_attr_parser::parse_arch): > Take cmdline_subset_list instead of current_subset_list when clone. > (riscv_process_target_attr): Record the func target info to hash > table. > (riscv_option_valid_attribute_p): Add new arg tree fndel. > * config/riscv/riscv.cc (riscv_declare_function_name): Consume the > func target info and print the arch message. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rvv/base/pr114352-3.c: New test. > > Signed-off-by: Pan Li <pan2...@intel.com> > --- > gcc/common/config/riscv/riscv-common.cc | 105 +++++++++++++++- > gcc/config/riscv/riscv-subset.h | 4 + > gcc/config/riscv/riscv-target-attr.cc | 18 ++- > gcc/config/riscv/riscv.cc | 7 +- > .../gcc.target/riscv/rvv/base/pr114352-3.c | 113 ++++++++++++++++++ > 5 files changed, 240 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr114352-3.c > > diff --git a/gcc/common/config/riscv/riscv-common.cc > b/gcc/common/config/riscv/riscv-common.cc > index d32bf147eca..76ec9bf846c 100644 > --- a/gcc/common/config/riscv/riscv-common.cc > +++ b/gcc/common/config/riscv/riscv-common.cc > @@ -425,11 +425,108 @@ bool riscv_subset_list::parse_failed = false; > > static riscv_subset_list *current_subset_list = NULL; > > +static riscv_subset_list *cmdline_subset_list = NULL; > + > +struct riscv_func_target_info > +{ > + tree fn_decl; > + std::string fn_target_name; > + > + riscv_func_target_info (const tree &decl, const std::string &target_name) > + : fn_decl (decl), fn_target_name (target_name) > + { > + } > +}; > + > +struct riscv_func_target_hasher : nofree_ptr_hash<struct > riscv_func_target_info> > +{ > + typedef tree compare_type; > + > + static hashval_t hash (value_type); > + static bool equal (value_type, const compare_type &); > +}; > + > +static hash_table<riscv_func_target_hasher> *func_target_table = NULL; > + > +static inline hashval_t riscv_func_decl_hash (tree fn_decl) > +{ > + inchash::hash h; > + > + h.add_ptr (fn_decl); > + > + return h.end (); > +} > + > +inline hashval_t > +riscv_func_target_hasher::hash (value_type value) > +{ > + return riscv_func_decl_hash (value->fn_decl); > +} > + > +inline bool > +riscv_func_target_hasher::equal (value_type value, const compare_type &key) > +{ > + return value->fn_decl == key; > +} > + > const riscv_subset_list *riscv_current_subset_list () > { > return current_subset_list; > } > > +const riscv_subset_list * riscv_cmdline_subset_list () > +{ > + return cmdline_subset_list; > +} > + > +static inline void riscv_func_target_table_lazy_init () > +{ > + if (func_target_table != NULL) > + return; > + > + func_target_table = new hash_table<riscv_func_target_hasher> (1023); > +} > + > +std::string * riscv_func_target_get (tree fn_decl) > +{ > + riscv_func_target_table_lazy_init (); > + > + hashval_t hash = riscv_func_decl_hash (fn_decl); > + struct riscv_func_target_info *info > + = func_target_table->find_with_hash (fn_decl, hash); > + > + return info == NULL ? NULL : &info->fn_target_name; > +} > + > +void riscv_func_target_put (tree fn_decl, std::string fn_target_name) > +{ > + riscv_func_target_table_lazy_init (); > + > + hashval_t hash = riscv_func_decl_hash (fn_decl); > + struct riscv_func_target_info **target_info_slot > + = func_target_table->find_slot_with_hash (fn_decl, hash, INSERT); > + > + gcc_assert (!*target_info_slot); > + > + struct riscv_func_target_info *info > + = new riscv_func_target_info (fn_decl, fn_target_name); > + > + *target_info_slot = info; > +} > + > +void riscv_func_target_remove_and_destory (tree fn_decl) > +{ > + hashval_t hash = riscv_func_decl_hash (fn_decl); > + struct riscv_func_target_info *info > + = func_target_table->find_with_hash (fn_decl, hash); > + > + if (info) > + { > + func_target_table->remove_elt_with_hash (fn_decl, hash); > + delete info; > + } > +} > + > /* struct for recording multi-lib info. */ > struct riscv_multi_lib_info_t { > std::string path; > @@ -1737,10 +1834,14 @@ riscv_parse_arch_string (const char *isa, > } > } > > - if (current_subset_list) > + /* Avoid double delete if current_subset_list equals cmdline_subset_list. > */ > + if (current_subset_list && current_subset_list != cmdline_subset_list) > delete current_subset_list; > > - current_subset_list = subset_list; > + if (cmdline_subset_list) > + delete cmdline_subset_list; > + > + current_subset_list = cmdline_subset_list = subset_list; > } > > /* Return the riscv_cpu_info entry for CPU, NULL if not found. */ > diff --git a/gcc/config/riscv/riscv-subset.h b/gcc/config/riscv/riscv-subset.h > index ae849e2a302..c4f1eaf4e26 100644 > --- a/gcc/config/riscv/riscv-subset.h > +++ b/gcc/config/riscv/riscv-subset.h > @@ -108,6 +108,10 @@ public: > }; > > extern const riscv_subset_list *riscv_current_subset_list (void); > +extern const riscv_subset_list *riscv_cmdline_subset_list (void); > +extern std::string * riscv_func_target_get (tree); > +extern void riscv_func_target_put (tree, std::string); > +extern void riscv_func_target_remove_and_destory (tree); > extern void > riscv_set_arch_by_subset_list (riscv_subset_list *, struct gcc_options *); > > diff --git a/gcc/config/riscv/riscv-target-attr.cc > b/gcc/config/riscv/riscv-target-attr.cc > index 9dbb78f28cc..2476250eed6 100644 > --- a/gcc/config/riscv/riscv-target-attr.cc > +++ b/gcc/config/riscv/riscv-target-attr.cc > @@ -54,6 +54,10 @@ public: > m_loc = loc; > } > > + riscv_subset_list* get_riscv_subset_list () { > + return m_subset_list; > + } > + > void update_settings (struct gcc_options *opts) const; > private: > const char *m_raw_attr_str; > @@ -109,7 +113,7 @@ riscv_target_attr_parser::parse_arch (const char *str) > char *str_to_check = buf.get (); > strcpy (str_to_check, str); > const char *token = strtok_r (str_to_check, ",", &str_to_check); > - m_subset_list = riscv_current_subset_list ()->clone (); > + m_subset_list = riscv_cmdline_subset_list ()->clone (); > m_subset_list->set_loc (m_loc); > while (token) > { > @@ -295,7 +299,8 @@ num_occurences_in_str (char c, char *str) > and update the global target options space. */ > > static bool > -riscv_process_target_attr (tree args, location_t loc, struct gcc_options > *opts) > +riscv_process_target_attr (tree fndecl, tree args, location_t loc, > + struct gcc_options *opts) > { > if (TREE_CODE (args) == TREE_LIST) > { > @@ -304,7 +309,7 @@ riscv_process_target_attr (tree args, location_t loc, > struct gcc_options *opts) > tree head = TREE_VALUE (args); > if (head) > { > - if (!riscv_process_target_attr (head, loc, opts)) > + if (!riscv_process_target_attr (fndecl, head, loc, opts)) > return false; > } > args = TREE_CHAIN (args); > @@ -357,6 +362,11 @@ riscv_process_target_attr (tree args, location_t loc, > struct gcc_options *opts) > /* Apply settings from target attribute. */ > attr_parser.update_settings (opts); > > + /* Add the string of the target attribute to the fndecl hash table. */ > + riscv_subset_list *subset_list = attr_parser.get_riscv_subset_list (); > + if (subset_list) > + riscv_func_target_put (fndecl, subset_list->to_string (true)); > + > return true; > } > > @@ -374,7 +384,7 @@ riscv_option_valid_attribute_p (tree fndecl, tree, tree > args, int) > /* Save the current target options to restore at the end. */ > cl_target_option_save (&cur_target, &global_options, &global_options_set); > > - ret = riscv_process_target_attr (args, loc, &global_options); > + ret = riscv_process_target_attr (fndecl, args, loc, &global_options); > > if (ret) > { > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc > index 89acb94af10..42bc3e6eeb3 100644 > --- a/gcc/config/riscv/riscv.cc > +++ b/gcc/config/riscv/riscv.cc > @@ -8673,8 +8673,13 @@ riscv_declare_function_name (FILE *stream, const char > *name, tree fndecl) > if (DECL_FUNCTION_SPECIFIC_TARGET (fndecl)) > { > fprintf (stream, "\t.option push\n"); > - std::string isa = riscv_current_subset_list ()->to_string (true); > + > + std::string *target_name = riscv_func_target_get (fndecl); > + std::string isa = target_name != NULL > + ? *target_name > + : riscv_cmdline_subset_list ()->to_string (true); > fprintf (stream, "\t.option arch, %s\n", isa.c_str ()); > + riscv_func_target_remove_and_destory (fndecl); > > struct cl_target_option *local_cl_target = > TREE_TARGET_OPTION (DECL_FUNCTION_SPECIFIC_TARGET (fndecl)); > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr114352-3.c > b/gcc/testsuite/gcc.target/riscv/rvv/base/pr114352-3.c > new file mode 100644 > index 00000000000..e7af4223d6a > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr114352-3.c > @@ -0,0 +1,113 @@ > +/* { dg-do compile } */ > +/* { dg-options "-march=rv64gc -mabi=lp64 -O3 -fno-schedule-insns > -fno-schedule-insns2" } */ > +/* { dg-final { check-function-bodies "**" "" } } */ > + > +/* > +** test_1: > +** sext\.w\s+[atx][0-9]+,\s*[atx][0-9]+ > +** ... > +*/ > +void > +test_1 (int *a, int *b, int *out, unsigned count) > +{ > + unsigned i; > + > + count = count > 128 ? 128 : count; > + > + for (i = 0; i < count; i++) > + out[i] = a[i] + b[i]; > +} > + > +/* > +** test_2: > +** ... > +** vadd\.vv\s+v[0-9]+,\s*v[0-9]+,\s*v[0-9]+ > +** ... > +*/ > +void > +__attribute__((target("arch=+v"))) > +test_2 (int *a, int *b, int *out, unsigned count) > +{ > + unsigned i; > + > + count = count > 128 ? 128 : count; > + > + for (i = 0; i < count; i++) > + out[i] = a[i] + b[i]; > +} > + > +/* > +** test_3: > +** ... > +** minu\s+[atx][0-9]+,\s*[atx][0-9]+,\s*[atx][0-9]+ > +** ... > +*/ > +void > +__attribute__((target("arch=+zbb"))) > +test_3 (int *a, int *b, int *out, unsigned count) > +{ > + unsigned i; > + > + count = count > 128 ? 128 : count; > + > + for (i = 0; i < count; i++) > + out[i] = a[i] + b[i]; > +} > + > +/* > +** test_4: > +** sext\.w\s+[atx][0-9]+,\s*[atx][0-9]+ > +** ... > +*/ > +void > +test_4 (int *a, int *b, int *out, unsigned count) > +{ > + unsigned i; > + > + count = count > 128 ? 128 : count; > + > + for (i = 0; i < count; i++) > + out[i] = a[i] + b[i]; > +} > + > +/* > +** test_5: > +** ... > +** fadd\.h\s+fa[0-9]+,\s*fa[0-9]+,\s*fa[0-9]+ > +** ... > +*/ > +void > +__attribute__((target("arch=+zfh"))) > +test_5 (_Float16 *a, _Float16 *b, _Float16 *out, unsigned count) > +{ > + unsigned i; > + > + count = count > 128 ? 128 : count; > + > + for (i = 0; i < count; i++) > + out[i] = a[i] + b[i]; > +} > + > +/* > +** test_6: > +** ... > +** call\s+__extendhfsf2 > +** ... > +** call\s+__truncsfhf2 > +** ... > +*/ > +void > +test_6 (_Float16 *a, _Float16 *b, _Float16 *out, unsigned count) > +{ > + unsigned i; > + > + count = count > 128 ? 128 : count; > + > + for (i = 0; i < count; i++) > + out[i] = a[i] + b[i]; > +} > + > +/* { dg-final { scan-assembler ".attribute arch, > \"rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zicsr2p0_zifencei2p0\"" } } */ > +/* { dg-final { scan-assembler ".option arch, > rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_v1p0_zicsr2p0_zifencei2p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0" > } } */ > +/* { dg-final { scan-assembler ".option arch, > rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zicsr2p0_zifencei2p0_zbb1p0" } } */ > +/* { dg-final { scan-assembler ".option arch, > rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zicsr2p0_zifencei2p0_zfh1p0_zfhmin1p0" } } > */ > -- > 2.34.1 >
