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. >> >
