[PATCH] predcom: Refactor more using auto_vec

[Kewen.Lin]

on 2021/7/20 上午4:45, Martin Sebor wrote:
> On 7/19/21 12:28 AM, Kewen.Lin wrote:
>> Hi Martin & Richard,
>>
>>>> A further improvement worth considering (if you're so inclined :)
>>>> is replacing the pcom_worker vec members with auto_vec (obviating
>>>> having to explicitly release them) and for the same reason also
>>>> replacing the comp_ptrs bare pointer members with auto_vecs.
>>>> There may be other opportunities to do the same in individual
>>>> functions (I'd look to get rid of as many calls to functions
>>>> like XNEW()/XNEWVEC() and free() use auto_vec instead).
>>>>
>>>> An unrelated but worthwhile change is to replace the FOR_EACH_
>>>> loops with C++ 11 range loops, analogously to:
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572315.html
>>>>
>>>> Finally, the only loosely followed naming convention for member
>>>> variables is to start them with the m_ prefix.
>>>>
>>>> These just suggestions that could be done in a followup, not
>>>> something I would consider prerequisite for accepting the patch
>>>> as is if I were in a position to make such a decision.
>>>>
>>
>> Sorry for the late update, this patch follows your previous
>> advices to refactor it more by:
>>    - Adding m_ prefix for class pcom_worker member variables.
>>    - Using auto_vec instead of vec among class pcom_worker,
>>      chain, component and comp_ptrs.
>>
>> btw, the changes in tree-data-ref.[ch] is required, without
>> it the destruction of auto_vec instance could try to double
>> free the memory pointed by m_vec.
> 
> Making the vec parameters in tree-data-ref.[ch] references is in line
> with other changes like those that we have agreed on in a separate
> review recently, so they look good to me.
> 

Nice, thanks for the information.

>>
>> The suggestion on range loops is addressed by one separated
>> patch: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575536.html
>>
>> Bootstrapped and regtested on powerpc64le-linux-gnu P9,
>> x86_64-redhat-linux and aarch64-linux-gnu, also
>> bootstrapped on ppc64le P9 with bootstrap-O3 config.
>>
>> Is it ok for trunk?
> 
> Thanks for taking the suggestions!  At a high-level the patch looks
> good.  I spotted a couple of new instances of XDELETE that I couldn't
> find corresponding XNEW() calls for but I'll leave that to someone
> more familiar with the code, along with a formal review and approval.
> 

The new XDELETEs are for struct component releasing, which uses "free"
in removed function release_component before.  As its original "new"
adopts "XCNEW" as below:

  comp = XCNEW (struct component);

I thought it might be good to use XDELETE to match when I touched it.

BR,
Kewen

> Martin
> 
>>
>> BR,
>> Kewen
>> -----
>> gcc/ChangeLog:
>>
>>     * tree-data-ref.c (free_dependence_relations): Adjust to pass vec by
>>     reference.
>>     (free_data_refs): Likewise.
>>     * tree-data-ref.h (free_dependence_relations): Likewise.
>>     (free_data_refs): Likewise.
>>     * tree-predcom.c (struct chain): Use auto_vec instead of vec for
>>     members.
>>     (struct component): Likewise.
>>     (pcom_worker::pcom_worker): Adjust for auto_vec and renaming changes.
>>     (pcom_worker::~pcom_worker): Likewise.
>>     (pcom_worker::release_chain): Adjust as auto_vec changes.
>>     (pcom_worker::loop): Rename to ...
>>     (pcom_worker::m_loop): ... this.
>>     (pcom_worker::datarefs): Rename to ...
>>     (pcom_worker::m_datarefs): ... this.  Use auto_vec instead of vec.
>>     (pcom_worker::dependences): Rename to ...
>>     (pcom_worker::m_dependences): ... this.  Use auto_vec instead of vec.
>>     (pcom_worker::chains): Rename to ...
>>     (pcom_worker::m_chains): ... this.  Use auto_vec instead of vec.
>>     (pcom_worker::looparound_phis): Rename to ...
>>     (pcom_worker::m_looparound_phis): ... this.  Use auto_vec instead of
>>     vec.
>>     (pcom_worker::cache): Rename to ...
>>     (pcom_worker::m_cache): ... this.  Use auto_vec instead of vec.
>>     (pcom_worker::release_chain): Adjust for auto_vec changes.
>>     (pcom_worker::release_chains): Adjust for auto_vec and renaming
>>     changes.
>>     (release_component): Remove.
>>     (release_components): Adjust for release_component removal.
>>     (component_of): Adjust to use vec.
>>     (merge_comps): Likewise.
>>     (pcom_worker::aff_combination_dr_offset): Adjust for renaming changes.
>>     (pcom_worker::determine_offset): Likewise.
>>     (class comp_ptrs): Remove.
>>     (pcom_worker::split_data_refs_to_components): Adjust for renaming
>>     changes, for comp_ptrs removal with auto_vec.
>>     (pcom_worker::suitable_component_p): Adjust for renaming changes.
>>     (pcom_worker::filter_suitable_components): Adjust for release_component
>>     removal.
>>     (pcom_worker::valid_initializer_p): Adjust for renaming changes.
>>     (pcom_worker::find_looparound_phi): Likewise.
>>     (pcom_worker::add_looparound_copies): Likewise.
>>     (pcom_worker::determine_roots_comp): Likewise.
>>     (pcom_worker::single_nonlooparound_use): Likewise.
>>     (pcom_worker::execute_pred_commoning_chain): Likewise.
>>     (pcom_worker::execute_pred_commoning): Likewise.
>>     (pcom_worker::try_combine_chains): Likewise.
>>     (pcom_worker::prepare_initializers_chain): Likewise.
>>     (pcom_worker::prepare_initializers): Likewise.
>>     (pcom_worker::prepare_finalizers_chain): Likewise.
>>     (pcom_worker::prepare_finalizers): Likewise.
>>     (pcom_worker::tree_predictive_commoning_loop): Likewise.
>>
>