Re: [PATCH v2] ipa-inline: Add target info into fn summary [PR102059]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Bill,

on 2021/9/13 上午12:34, Bill Schmidt wrote:
> Hi Kewen,
> 
> I'll leave the continued review of the back-end parts of this to Segher, but I do have one long-term comment.  The rs6000_builtin_info[code].mask field that you're relying on is going away as part of the built-in function rewrite.  There will be a "bifattrs" field that replaces this in the table-driven data structures.  I'm including those below.  Please think about whether what you're building will easily translate to using these data structures in the near future.  I don't think there are any show-stoppers here, but if the new table needs adjustment to make your changes easier, let's discuss.
> 

Thanks for pointing this out and the detailed related information.  I installed your patch series
"[PATCHv5 00/18] Replace the Power target-specific builtin machinery" and found that the existing
interface bif_is_htm works perfectly and nothing need to be added for the HTM check here.
Thanks for the efforts!  btw, the diff I used for testing is listed below:

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index f90a5f40e4a..eee04fc21f2 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -25495,13 +25495,26 @@ rs6000_update_ipa_fn_target_info (vec<HOST_WIDE_INT> &info, const gimple *stmt)
       tree fndecl = gimple_call_fndecl (stmt);
       if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_MD))
 	{
-	  enum rs6000_builtins fcode =
-	    (enum rs6000_builtins) DECL_MD_FUNCTION_CODE (fndecl);
-	  /* HTM bifs definitely exploit HTM insns.  */
-	  if (rs6000_fn_has_any_of_these_mask_bits (fcode, RS6000_BTM_HTM))
+	  if (new_builtins_are_live)
 	    {
-	      info[0] |= OPTION_MASK_HTM;
-	      return false;
+	      enum rs6000_gen_builtins fcode =
+		(enum rs6000_gen_builtins) DECL_MD_FUNCTION_CODE (fndecl);
+	      if (bif_is_htm (rs6000_builtin_info_x[fcode]))
+		{
+		  info[0] |= OPTION_MASK_HTM;
+		  return false;
+		}
+	    }
+	  else
+	    {
+	      enum rs6000_builtins fcode =
+		(enum rs6000_builtins) DECL_MD_FUNCTION_CODE (fndecl);
+	      /* HTM bifs definitely exploit HTM insns.  */
+	      if (rs6000_fn_has_any_of_these_mask_bits (fcode, RS6000_BTM_HTM))
+		{
+		  info[0] |= OPTION_MASK_HTM;
+		  return false;
+		}
 	    }
 	}
     }


BR,
Kewen

> Thanks,
> Bill
> 
> #define PPC_MAXRESTROPNDS 3
> struct GTY((user)) bifdata
> {
>   const char *bifname;
>   bif_enable enable;
>   tree fntype;
>   insn_code icode;
>   int  nargs;
>   int  bifattrs;
>   int  restr_opnd[PPC_MAXRESTROPNDS];
>   restriction restr[PPC_MAXRESTROPNDS];
>   int  restr_val1[PPC_MAXRESTROPNDS];
>   int  restr_val2[PPC_MAXRESTROPNDS];
>   const char *attr_string;
>   rs6000_gen_builtins assoc_bif;
> };
> 
> #define bif_init_bit            (0x00000001)
> #define bif_set_bit             (0x00000002)
> #define bif_extract_bit         (0x00000004)
> #define bif_nosoft_bit          (0x00000008)
> #define bif_ldvec_bit           (0x00000010)
> #define bif_stvec_bit           (0x00000020)
> #define bif_reve_bit            (0x00000040)
> #define bif_pred_bit            (0x00000080)
> #define bif_htm_bit             (0x00000100)
> #define bif_htmspr_bit          (0x00000200)
> #define bif_htmcr_bit           (0x00000400)
> #define bif_mma_bit             (0x00000800)
> #define bif_quad_bit            (0x00001000)
> #define bif_pair_bit            (0x00002000)
> #define bif_mmaint_bit          (0x00004000)
> #define bif_no32bit_bit         (0x00008000)
> #define bif_32bit_bit           (0x00010000)
> #define bif_cpu_bit             (0x00020000)
> #define bif_ldstmask_bit        (0x00040000)
> #define bif_lxvrse_bit          (0x00080000)
> #define bif_lxvrze_bit          (0x00100000)
> #define bif_endian_bit          (0x00200000)
> 
> #define bif_is_init(x)          ((x).bifattrs & bif_init_bit)
> #define bif_is_set(x)           ((x).bifattrs & bif_set_bit)
> #define bif_is_extract(x)       ((x).bifattrs & bif_extract_bit)
> #define bif_is_nosoft(x)        ((x).bifattrs & bif_nosoft_bit)
> #define bif_is_ldvec(x)         ((x).bifattrs & bif_ldvec_bit)
> #define bif_is_stvec(x)         ((x).bifattrs & bif_stvec_bit)
> #define bif_is_reve(x)          ((x).bifattrs & bif_reve_bit)
> #define bif_is_predicate(x)     ((x).bifattrs & bif_pred_bit)
> #define bif_is_htm(x)           ((x).bifattrs & bif_htm_bit)
> #define bif_is_htmspr(x)        ((x).bifattrs & bif_htmspr_bit)
> #define bif_is_htmcr(x)         ((x).bifattrs & bif_htmcr_bit)
> #define bif_is_mma(x)           ((x).bifattrs & bif_mma_bit)
> #define bif_is_quad(x)          ((x).bifattrs & bif_quad_bit)
> #define bif_is_pair(x)          ((x).bifattrs & bif_pair_bit)
> #define bif_is_mmaint(x)                ((x).bifattrs & bif_mmaint_bit)
> #define bif_is_no32bit(x)       ((x).bifattrs & bif_no32bit_bit)
> #define bif_is_32bit(x) ((x).bifattrs & bif_32bit_bit)
> #define bif_is_cpu(x)           ((x).bifattrs & bif_cpu_bit)
> #define bif_is_ldstmask(x)      ((x).bifattrs & bif_ldstmask_bit)
> #define bif_is_lxvrse(x)        ((x).bifattrs & bif_lxvrse_bit)
> #define bif_is_lxvrze(x)        ((x).bifattrs & bif_lxvrze_bit)
> #define bif_is_endian(x)        ((x).bifattrs & bif_endian_bit)
> 
> extern bifdata rs6000_builtin_info_x[RS6000_BIF_MAX];
> 
> On 9/8/21 2:43 AM, Kewen.Lin wrote:
>> Hi!
>>
>> Power ISA 2.07 (Power8) introduces transactional memory feature
>> but ISA3.1 (Power10) removes it.  It exposes one troublesome
>> issue as PR102059 shows.  Users define some function with
>> target pragma cpu=power10 then it calls one function with
>> attribute always_inline which inherits command line option
>> cpu=power8 which enables HTM implicitly.  The current isa_flags
>> check doesn't allow this inlining due to "target specific
>> option mismatch" and error mesasge is emitted.
>>
>> Normally, the callee function isn't intended to exploit HTM
>> feature, but the default flag setting make it look it has.
>> As Richi raised in the PR, we have fp_expressions flag in
>> function summary, and allow us to check the function actually
>> contains any floating point expressions to avoid overkill.
>> So this patch follows the similar idea but is more target
>> specific, for this rs6000 port specific requirement on HTM
>> feature check, we would like to check rs6000 specific HTM
>> built-in functions and inline assembly, it allows targets
>> to do their own customized checks and updates.
>>
>> It introduces two target hooks need_ipa_fn_target_info and
>> update_ipa_fn_target_info.  The former allows target to do
>> some previous check and decides to collect target specific
>> information for this function or not.  For some special case,
>> it can predict the analysis result and push it early without
>> any scannings.  The latter allows the analyze_function_body
>> to pass gimple stmts down just like fp_expressions handlings,
>> target can do its own tricks.  I put them as one hook initially
>> with one boolean to indicates whether it's initial time, but
>> the code looks a bit ugly, to separate them seems to have
>> better readability.
>>
>> To make it simple, this patch uses HOST_WIDE_INT to record the
>> flags just like what we use for isa_flags.  For rs6000's HTM
>> need, one HOST_WIDE_INT variable is quite enough, but it seems
>> good to have one auto_vec for scalability as I noticed some
>> targets have more than one HOST_WIDE_INT flag.
>>
>> Against v1 [1], this v2 addressed Richi's and Segher's review
>> comments, mainly consists of:
>>   - Extend it to cover non always_inline.
>>   - Exclude the case for offload streaming.
>>   - Some function naming and formatting issues.
>>   - Adjust rs6000_can_inline_p.
>>   - Add new cases.
>>
>> The patch has been bootstrapped and regress-tested on
>> powerpc64le-linux-gnu Power9.
>>
>> Any comments are highly appreciated!
>>
>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-September/578555.html
>>
>> BR,
>> Kewen
>> -----
>> gcc/ChangeLog:
>>
>> 	PR ipa/102059
>> 	* config/rs6000/rs6000-call.c (rs6000_fn_has_any_of_these_mask_bits):
>> 	New function.
>> 	* config/rs6000/rs6000-internal.h
>> 	(rs6000_fn_has_any_of_these_mask_bits): New declare.
>> 	* config/rs6000/rs6000.c (TARGET_NEED_IPA_FN_TARGET_INFO): New macro.
>> 	(TARGET_UPDATE_IPA_FN_TARGET_INFO): Likewise.
>> 	(rs6000_need_ipa_fn_target_info): New function.
>> 	(rs6000_update_ipa_fn_target_info): Likewise.
>> 	(rs6000_can_inline_p): Adjust for ipa function summary target info.
>> 	* ipa-fnsummary.c (ipa_dump_fn_summary): Adjust for ipa function
>> 	summary target info.
>> 	(analyze_function_body): Adjust for ipa function summary target
>> 	info and call hook rs6000_need_ipa_fn_target_info and
>> 	rs6000_update_ipa_fn_target_info.
>> 	(ipa_merge_fn_summary_after_inlining): Adjust for ipa function
>> 	summary target info.
>> 	(inline_read_section): Likewise.
>> 	(ipa_fn_summary_write): Likewise.
>> 	* ipa-fnsummary.h (ipa_fn_summary::target_info): New member.
>> 	* doc/tm.texi: Regenerate.
>> 	* doc/tm.texi.in (TARGET_UPDATE_IPA_FN_TARGET_INFO): Document new
>> 	hook.
>> 	(TARGET_NEED_IPA_FN_TARGET_INFO): Likewise.
>> 	* target.def (update_ipa_fn_target_info): New hook.
>> 	(need_ipa_fn_target_info): Likewise.
>> 	* targhooks.c (default_need_ipa_fn_target_info): New function.
>> 	(default_update_ipa_fn_target_info): Likewise.
>> 	* targhooks.h (default_update_ipa_fn_target_info): New declare.
>> 	(default_need_ipa_fn_target_info): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR ipa/102059
>> 	* gcc.dg/lto/pr102059-1_0.c: New test.
>> 	* gcc.dg/lto/pr102059-1_1.c: New test.
>> 	* gcc.dg/lto/pr102059-1_2.c: New test.
>> 	* gcc.dg/lto/pr102059-2_0.c: New test.
>> 	* gcc.dg/lto/pr102059-2_1.c: New test.
>> 	* gcc.dg/lto/pr102059-2_2.c: New test.
>> 	* gcc.target/powerpc/pr102059-5.c: New test.
>> 	* gcc.target/powerpc/pr102059-6.c: New test.
>> 	* gcc.target/powerpc/pr102059-7.c: New test.