On 6/21/21 8:35 PM, Kewen.Lin wrote:
Hi Richi and Martin,
Thanks Richi! One draft (not ready for review) is attached for the further
discussion. It follows the idea of RAII-style cleanup. I noticed that
Martin suggested stepping forward to make tree_predictive_commoning_loop
and its callees into one class (Thanks Martin), since there are not many
this kind of C++-style work functions, I want to double confirm which option
do you guys prefer?
Such general cleanup is of course desired - Giuliano started some of it within
GSoC two years ago in the attempt to thread the compilation process. The
cleanup then helps to get rid of global state which of course interferes here
(and avoids unnecessary use of TLS vars).
So yes, encapsulating global state into a class and making accessors
member functions is something that is desired (but a lot of mechanical
work).
Thanks
Richard.
I meant that not necessarily as something to include in this patch
but as a suggestion for a future improvement. If you'd like to
tackle it at any point that would be great of course In any
event, thanks for double-checking!
The attached patch looks good to me as well (more for the sake of
style than anything else, declaring the class copy ctor and copy assignment =
delete would > make it clear it's not meant to be
copied, although in this case it's unlikely to make a practical
difference).
Martin.
Thanks for your explanation! Sorry for the late response.
As the way to encapsulate global state into a class and making accessors
member functions looks more complete, I gave up the RAII draft and
switched onto this way.
This patch is to encapsulate global states into a class and
making their accessors as member functions, remove some
consequent useless clean up code, and do some clean up with
RAII.
Nice!
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.
Martin
Bootstrapped/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?
BR,
Kewen
-----
gcc/ChangeLog:
* tree-predcom.c (class pcom_worker): New class.
(release_chain): Renamed to...
(pcom_worker::release_chain): ...this.
(release_chains): Renamed to...
(pcom_worker::release_chains): ...this.
(aff_combination_dr_offset): Renamed to...
(pcom_worker::aff_combination_dr_offset): ...this.
(determine_offset): Renamed to...
(pcom_worker::determine_offset): ...this.
(class comp_ptrs): New class.
(split_data_refs_to_components): Renamed to...
(pcom_worker::split_data_refs_to_components): ...this,
and update with class comp_ptrs.
(suitable_component_p): Renamed to...
(pcom_worker::suitable_component_p): ...this.
(filter_suitable_components): Renamed to...
(pcom_worker::filter_suitable_components): ...this.
(valid_initializer_p): Renamed to...
(pcom_worker::valid_initializer_p): ...this.
(find_looparound_phi): Renamed to...
(pcom_worker::find_looparound_phi): ...this.
(add_looparound_copies): Renamed to...
(pcom_worker::add_looparound_copies): ...this.
(determine_roots_comp): Renamed to...
(pcom_worker::determine_roots_comp): ...this.
(determine_roots): Renamed to...
(pcom_worker::determine_roots): ...this.
(single_nonlooparound_use): Renamed to...
(pcom_worker::single_nonlooparound_use): ...this.
(remove_stmt): Renamed to...
(pcom_worker::remove_stmt): ...this.
(execute_pred_commoning_chain): Renamed to...
(pcom_worker::execute_pred_commoning_chain): ...this.
(execute_pred_commoning): Renamed to...
(pcom_worker::execute_pred_commoning): ...this.
(struct epcc_data): New member worker.
(execute_pred_commoning_cbck): Call execute_pred_commoning
with pcom_worker pointer.
(find_use_stmt): Renamed to...
(pcom_worker::find_use_stmt): ...this.
(find_associative_operation_root): Renamed to...
(pcom_worker::find_associative_operation_root): ...this.
(find_common_use_stmt): Renamed to...
(pcom_worker::find_common_use_stmt): ...this.
(combinable_refs_p): Renamed to...
(pcom_worker::combinable_refs_p): ...this.
(reassociate_to_the_same_stmt): Renamed to...
(pcom_worker::reassociate_to_the_same_stmt): ...this.
(stmt_combining_refs): Renamed to...
(pcom_worker::stmt_combining_refs): ...this.
(combine_chains): Renamed to...
(pcom_worker::combine_chains): ...this.
(try_combine_chains): Renamed to...
(pcom_worker::try_combine_chains): ...this.
(prepare_initializers_chain): Renamed to...
(pcom_worker::prepare_initializers_chain): ...this.
(prepare_initializers): Renamed to...
(pcom_worker::prepare_initializers): ...this.
(prepare_finalizers_chain): Renamed to...
(pcom_worker::prepare_finalizers_chain): ...this.
(prepare_finalizers): Renamed to...
(pcom_worker::prepare_finalizers): ...this.
(tree_predictive_commoning_loop): Renamed to...
(pcom_worker::tree_predictive_commoning_loop): ...this, adjust
some calls and remove some cleanup code.
(tree_predictive_commoning): Adjusted to use pcom_worker instance.
(static variable looparound_phis): Remove.
(static variable name_expansions): Remove.
