Re: predcom: Refactor more by encapsulating global states

[Martin Sebor <msebor@gmail.com>]

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