> Am 31.07.2025 um 17:43 schrieb Tamar Christina <tamar.christ...@arm.com>:
> 
> 
>> 
>> -----Original Message-----
>> From: Richard Biener <rguent...@suse.de>
>> Sent: Thursday, July 31, 2025 3:09 PM
>> To: gcc-patches@gcc.gnu.org
>> Cc: Tamar Christina <tamar.christ...@arm.com>
>> Subject: [PATCH 2/2] Put SLP_TREE_SIMD_CLONE_INFO into type specifc data
>> 
>> The following adds vect_simd_clone_data as a container for vect
>> type specific data for vectorizable_simd_clone_call and moves
>> SLP_TREE_SIMD_CLONE_INFO there.
>> 
>> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>> 
>> Any further considerations?
>> 
> 
> Looks much better now!
> 
>> Thanks,
>> Richard.
>> 
>>    * tree-vectorizer.h (vect_simd_clone_data): New.
>>    (_slp_tree::simd_clone_info): Remove.
>>    (SLP_TREE_SIMD_CLONE_INFO): Likewise.
>>    * tree-vect-slp.cc (_slp_tree::_slp_tree): Adjust.
>>    (_slp_tree::~_slp_tree): Likewise.
>>    * tree-vect-stmts.cc (vectorizable_simd_clone_call): Use
>>    tyupe specific data to store SLP_TREE_SIMD_CLONE_INFO.
>> ---
>> gcc/tree-vect-slp.cc   |  2 --
>> gcc/tree-vect-stmts.cc |  7 ++++---
>> gcc/tree-vectorizer.h  | 19 +++++++++++++------
>> 3 files changed, 17 insertions(+), 11 deletions(-)
>> 
>> diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
>> index 233543214fa..4038fe84e87 100644
>> --- a/gcc/tree-vect-slp.cc
>> +++ b/gcc/tree-vect-slp.cc
>> @@ -118,7 +118,6 @@ _slp_tree::_slp_tree ()
>>   SLP_TREE_CHILDREN (this) = vNULL;
>>   SLP_TREE_LOAD_PERMUTATION (this) = vNULL;
>>   SLP_TREE_LANE_PERMUTATION (this) = vNULL;
>> -  SLP_TREE_SIMD_CLONE_INFO (this) = vNULL;
>>   SLP_TREE_DEF_TYPE (this) = vect_uninitialized_def;
>>   SLP_TREE_CODE (this) = ERROR_MARK;
>>   this->ldst_lanes = false;
>> @@ -150,7 +149,6 @@ _slp_tree::~_slp_tree ()
>>   SLP_TREE_VEC_DEFS (this).release ();
>>   SLP_TREE_LOAD_PERMUTATION (this).release ();
>>   SLP_TREE_LANE_PERMUTATION (this).release ();
>> -  SLP_TREE_SIMD_CLONE_INFO (this).release ();
>>   if (this->failed)
>>     free (failed);
>>   if (this->data)
>> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
>> index 3fa4585160c..24a3030bd9f 100644
>> --- a/gcc/tree-vect-stmts.cc
>> +++ b/gcc/tree-vect-stmts.cc
>> @@ -3892,9 +3892,9 @@ vectorizable_simd_clone_call (vec_info *vinfo,
>> stmt_vec_info stmt_info,
>>   if (nargs == 0)
>>     return false;
>> 
>> -  vec<tree>& simd_clone_info = SLP_TREE_SIMD_CLONE_INFO (slp_node);
>> -  if (cost_vec)
>> -    simd_clone_info.truncate (0);
>> +  vect_simd_clone_data _data;
>> +  vect_simd_clone_data &data = slp_node->get_data (_data);
>> +  vec<tree>& simd_clone_info = data.simd_clone_info;
> 
> Yeah it's indeed a shame we can't avoid the local, we can always hide the 
> sequence
> Behind a macro, something like
> 
> EXPAND_SLP_DATA (data, slp_node)  which expands to
> 
> vect_simd_clone_data _data;
> vect_simd_clone_data &data = slp_node->get_data (_data);
> 
> but I honestly don't mind seeing the expansion explicitly.

Yeah.  In case we split the function into a analysis and transform one this 
tiny ugliness would go away.

>>   arginfo.reserve (nargs, true);
>>   auto_vec<slp_tree> slp_op;
>>   slp_op.safe_grow_cleared (nargs);
>> @@ -4291,6 +4291,7 @@ vectorizable_simd_clone_call (vec_info *vinfo,
>> stmt_vec_info stmt_info,
>>    }
>> 
>>       SLP_TREE_TYPE (slp_node) = call_simd_clone_vec_info_type;
>> +      slp_node->data = new vect_simd_clone_data (std::move (_data));
> 
> Do we still need the std::move here? doesn't the move constructor already
> take care of it?

Without it it complains I call the deleted non-move CTOR, so apparently it is 
necessary.

Richard 

> Otherwise looks good to me.
> 
> Thanks for the cleanup!
> 
> Tamar
> 
>>       DUMP_VECT_SCOPE ("vectorizable_simd_clone_call");
>> /*      vect_model_simple_cost (vinfo, 1, slp_node, cost_vec); */
>>       return true;
>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
>> index 63ba7c1e8f5..b8312b8d6bd 100644
>> --- a/gcc/tree-vectorizer.h
>> +++ b/gcc/tree-vectorizer.h
>> @@ -243,6 +243,19 @@ struct vect_data {
>>   virtual ~vect_data () = default;
>> };
>> 
>> +/* Analysis data from vectorizable_simd_clone_call for
>> +   call_simd_clone_vec_info_type.  */
>> +struct vect_simd_clone_data : vect_data {
>> +  virtual ~vect_simd_clone_data () = default;
>> +  vect_simd_clone_data () = default;
>> +  vect_simd_clone_data (vect_simd_clone_data &&other) = default;
>> +
>> +  /* Selected SIMD clone's function info.  First vector element
>> +     is SIMD clone's function decl, followed by a pair of trees (base + 
>> step)
>> +     for linear arguments (pair of NULLs for other arguments).  */
>> +  auto_vec<tree> simd_clone_info;
>> +};
>> +
>> /* A computation tree of an SLP instance.  Each node corresponds to a group 
>> of
>>    stmts to be packed in a SIMD stmt.  */
>> struct _slp_tree {
>> @@ -271,11 +284,6 @@ struct _slp_tree {
>>      denotes the number of output lanes.  */
>>   lane_permutation_t lane_permutation;
>> 
>> -  /* Selected SIMD clone's function info.  First vector element
>> -     is SIMD clone's function decl, followed by a pair of trees (base + 
>> step)
>> -     for linear arguments (pair of NULLs for other arguments).  */
>> -  vec<tree> simd_clone_info;
>> -
>>   tree vectype;
>>   /* Vectorized defs.  */
>>   vec<tree> vec_defs;
>> @@ -395,7 +403,6 @@ public:
>> #define SLP_TREE_NUMBER_OF_VEC_STMTS(S)          (S)->vec_stmts_size
>> #define SLP_TREE_LOAD_PERMUTATION(S)             (S)->load_permutation
>> #define SLP_TREE_LANE_PERMUTATION(S)             (S)->lane_permutation
>> -#define SLP_TREE_SIMD_CLONE_INFO(S)              (S)->simd_clone_info
>> #define SLP_TREE_DEF_TYPE(S)             (S)->def_type
>> #define SLP_TREE_VECTYPE(S)             (S)->vectype
>> #define SLP_TREE_REPRESENTATIVE(S)         (S)->representative
>> --
>> 2.43.0

Reply via email to