Re: [Patch, aarch64] v3: Preparatory patch to place target independent and,dependent changed code in one file
Hello Alex:
On 13/05/24 8:49 pm, Alex Coplan wrote:
> Hi Ajit,
>
> Why did you send three mails for this revision of the patch? If you're
> going to send a new revision of the patch you should increment the
> version number and outline the changes / reasons for the new revision.
>
There were issues sending the patch through thunderbird, hence multople
pacthes. Sorry for inconvenience caused.
> Mostly the comments below are just style nits and things you missed from
> the last review(s) (please try not to miss so many in the future).
>
Addressed.
> On 09/05/2024 17:06, Ajit Agarwal wrote:
>> Hello Alex/Richard:
>>
>> All review comments are addressed.
>>
>> Common infrastructure of load store pair fusion is divided into target
>> independent and target dependent changed code.
>>
>> Target independent code is the Generic code with pure virtual function
>> to interface betwwen target independent and dependent code.
>>
>> Target dependent code is the implementation of pure virtual function for
>> aarch64 target and the call to target independent code.
>>
>> Bootstrapped on aarch64-linux-gnu.
>>
>> Thanks & Regards
>> Ajit
>>
>>
>>
>> aarch64: Preparatory patch to place target independent and
>> dependent changed code in one file
>>
>> Common infrastructure of load store pair fusion is divided into target
>> independent and target dependent changed code.
>>
>> Target independent code is the Generic code with pure virtual function
>> to interface betwwen target independent and dependent code.
>>
>> Target dependent code is the implementation of pure virtual function for
>> aarch64 target and the call to target independent code.
>>
>> 2024-05-09 Ajit Kumar Agarwal
>>
>> gcc/ChangeLog:
>>
>> * config/aarch64/aarch64-ldp-fusion.cc: Place target
>> independent and dependent changed code.
>> ---
>> gcc/config/aarch64/aarch64-ldp-fusion.cc | 542 +++
>> 1 file changed, 363 insertions(+), 179 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> index 1d9caeab05d..217790e111a 100644
>> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> @@ -138,6 +138,224 @@ struct alt_base
>>poly_int64 offset;
>> };
>>
>> +// Virtual base class for load/store walkers used in alias analysis.
>> +struct alias_walker
>> +{
>> + virtual bool conflict_p (int ) const = 0;
>> + virtual insn_info *insn () const = 0;
>> + virtual bool valid () const = 0;
>> + virtual void advance () = 0;
>> +};
>> +
>> +enum class writeback{
>
> You missed a nit here. Space before '{'.
>
Addressed.
>> + ALL,
>> + EXISTING
>> +};
>
> You also missed adding comments for the enum, please see the review for v2:
> https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651074.html
>
Addressed.
>> +
>> +struct pair_fusion {
>> + pair_fusion ()
>> + {
>> +calculate_dominance_info (CDI_DOMINATORS);
>> +df_analyze ();
>> +crtl->ssa = new rtl_ssa::function_info (cfun);
>> + };
>> +
>> + // Given:
>> + // - an rtx REG_OP, the non-memory operand in a load/store insn,
>> + // - a machine_mode MEM_MODE, the mode of the MEM in that insn, and
>> + // - a boolean LOAD_P (true iff the insn is a load), then:
>> + // return true if the access should be considered an FP/SIMD access.
>> + // Such accesses are segregated from GPR accesses, since we only want
>> + // to form pairs for accesses that use the same register file.
>> + virtual bool fpsimd_op_p (rtx, machine_mode, bool)
>> + {
>> +return false;
>> + }
>> +
>> + // Return true if we should consider forming ldp/stp insns from memory
>> + // accesses with operand mode MODE at this stage in compilation.
>> + virtual bool pair_operand_mode_ok_p (machine_mode mode) = 0;
>> +
>> + // Return true iff REG_OP is a suitable register operand for a paired
>> + // memory access, where LOAD_P is true if we're asking about loads and
>> + // false for stores. MEM_MODE gives the mode of the operand.
>> + virtual bool pair_reg_operand_ok_p (bool load_p, rtx reg_op,
>> + machine_mode mode) = 0;
>
> The comment needs updating since we changed the name of the last param,
> i.e. s/MEM_MODE/MODE/.
>
Addressed.
>> +
>> + // Return alias check limit.
>> + // This is needed to avoid unbounded quadratic behaviour when
>> + // performing alias analysis.
>> + virtual int pair_mem_alias_check_limit () = 0;
>> +
>> + // Returns true if we should try to handle writeback opportunities
>> + // (not whether there are any).
>> + virtual bool handle_writeback_opportunities (enum writeback which) = 0 ;
>
> Heh, the bit in parens from the v2 review probably doesn't need to go
> into the comment here.
>
> Also you should describe WHICH in the comment.
>
>> +
>> + // Given BASE_MEM, the mem from the lower candidate access for a pair,
>> + // and LOAD_P (true if the access is a load), check if we should
Re: [Patch, aarch64] v3: Preparatory patch to place target independent and,dependent changed code in one file
Hi Ajit,
Why did you send three mails for this revision of the patch? If you're
going to send a new revision of the patch you should increment the
version number and outline the changes / reasons for the new revision.
Mostly the comments below are just style nits and things you missed from
the last review(s) (please try not to miss so many in the future).
On 09/05/2024 17:06, Ajit Agarwal wrote:
> Hello Alex/Richard:
>
> All review comments are addressed.
>
> Common infrastructure of load store pair fusion is divided into target
> independent and target dependent changed code.
>
> Target independent code is the Generic code with pure virtual function
> to interface betwwen target independent and dependent code.
>
> Target dependent code is the implementation of pure virtual function for
> aarch64 target and the call to target independent code.
>
> Bootstrapped on aarch64-linux-gnu.
>
> Thanks & Regards
> Ajit
>
>
>
> aarch64: Preparatory patch to place target independent and
> dependent changed code in one file
>
> Common infrastructure of load store pair fusion is divided into target
> independent and target dependent changed code.
>
> Target independent code is the Generic code with pure virtual function
> to interface betwwen target independent and dependent code.
>
> Target dependent code is the implementation of pure virtual function for
> aarch64 target and the call to target independent code.
>
> 2024-05-09 Ajit Kumar Agarwal
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-ldp-fusion.cc: Place target
> independent and dependent changed code.
> ---
> gcc/config/aarch64/aarch64-ldp-fusion.cc | 542 +++
> 1 file changed, 363 insertions(+), 179 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> index 1d9caeab05d..217790e111a 100644
> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> @@ -138,6 +138,224 @@ struct alt_base
>poly_int64 offset;
> };
>
> +// Virtual base class for load/store walkers used in alias analysis.
> +struct alias_walker
> +{
> + virtual bool conflict_p (int ) const = 0;
> + virtual insn_info *insn () const = 0;
> + virtual bool valid () const = 0;
> + virtual void advance () = 0;
> +};
> +
> +enum class writeback{
You missed a nit here. Space before '{'.
> + ALL,
> + EXISTING
> +};
You also missed adding comments for the enum, please see the review for v2:
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651074.html
> +
> +struct pair_fusion {
> + pair_fusion ()
> + {
> +calculate_dominance_info (CDI_DOMINATORS);
> +df_analyze ();
> +crtl->ssa = new rtl_ssa::function_info (cfun);
> + };
> +
> + // Given:
> + // - an rtx REG_OP, the non-memory operand in a load/store insn,
> + // - a machine_mode MEM_MODE, the mode of the MEM in that insn, and
> + // - a boolean LOAD_P (true iff the insn is a load), then:
> + // return true if the access should be considered an FP/SIMD access.
> + // Such accesses are segregated from GPR accesses, since we only want
> + // to form pairs for accesses that use the same register file.
> + virtual bool fpsimd_op_p (rtx, machine_mode, bool)
> + {
> +return false;
> + }
> +
> + // Return true if we should consider forming ldp/stp insns from memory
> + // accesses with operand mode MODE at this stage in compilation.
> + virtual bool pair_operand_mode_ok_p (machine_mode mode) = 0;
> +
> + // Return true iff REG_OP is a suitable register operand for a paired
> + // memory access, where LOAD_P is true if we're asking about loads and
> + // false for stores. MEM_MODE gives the mode of the operand.
> + virtual bool pair_reg_operand_ok_p (bool load_p, rtx reg_op,
> + machine_mode mode) = 0;
The comment needs updating since we changed the name of the last param,
i.e. s/MEM_MODE/MODE/.
> +
> + // Return alias check limit.
> + // This is needed to avoid unbounded quadratic behaviour when
> + // performing alias analysis.
> + virtual int pair_mem_alias_check_limit () = 0;
> +
> + // Returns true if we should try to handle writeback opportunities
> + // (not whether there are any).
> + virtual bool handle_writeback_opportunities (enum writeback which) = 0 ;
Heh, the bit in parens from the v2 review probably doesn't need to go
into the comment here.
Also you should describe WHICH in the comment.
> +
> + // Given BASE_MEM, the mem from the lower candidate access for a pair,
> + // and LOAD_P (true if the access is a load), check if we should proceed
> + // to form the pair given the target's code generation policy on
> + // paired accesses.
> + virtual bool pair_mem_ok_with_policy (rtx first_mem, bool load_p,
> + machine_mode mode) = 0;
The name of the first param needs updating in the prototype, i.e.
s/first_mem/base_mem/. I think you missed the bit about
