Hello Richard:
On 31/05/24 3:23 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagar...@linux.ibm.com> writes:
>> Hello All:
>>
>> Common infrastructure using generic code for pair mem fusion of different
>> targets.
>>
>> rs6000 target specific specific code implements virtual functions defined
>> by generic code.
>>
>> Code is implemented with pure virtual functions to interface with target
>> code.
>>
>> Target specific code are added in rs6000-mem-fusion.cc and additional virtual
>> function implementation required for rs6000 are added in
>> aarch64-ldp-fusion.cc.
>>
>> Bootstrapped and regtested for aarch64-linux-gnu and powerpc64-linux-gnu.
>>
>> Thanks & Regards
>> Ajit
>>
>>
>> aarch64, rs6000, middle-end: Add implementation for different targets for
>> pair mem fusion
>>
>> Common infrastructure using generic code for pair mem fusion of different
>> targets.
>>
>> rs6000 target specific specific code implements virtual functions defined
>> by generic code.
>>
>> Code is implemented with pure virtual functions to interface with target
>> code.
>>
>> Target specific code are added in rs6000-mem-fusion.cc and additional virtual
>> function implementation required for rs6000 are added in
>> aarch64-ldp-fusion.cc.
>>
>> 2024-05-31 Ajit Kumar Agarwal <aagar...@linux.ibm.com>
>>
>> gcc/ChangeLog:
>>
>> * config/aarch64/aarch64-ldp-fusion.cc: Add target specific
>> implementation of additional virtual functions added in pair_fusion
>> struct.
>> * config/rs6000/rs6000-passes.def: New mem fusion pass
>> before pass_early_remat.
>> * config/rs6000/rs6000-mem-fusion.cc: Add new pass.
>> Add target specific implementation using pure virtual
>> functions.
>> * config.gcc: Add new object file.
>> * config/rs6000/rs6000-protos.h: Add new prototype for mem
>> fusion pass.
>> * config/rs6000/t-rs6000: Add new rule.
>> * rtl-ssa/accesses.h: Moved set_is_live_out_use as public
>> from private.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.target/powerpc/me-fusion.C: New test.
>> * g++.target/powerpc/mem-fusion-1.C: New test.
>> * gcc.target/powerpc/mma-builtin-1.c: Modify test.
>> ---
>
> This isn't a complete review, just some initial questions & comments
> about selected parts.
>
>> [...]
>> +/* Check whether load can be fusable or not.
>> + Return true if dependent use is UNSPEC otherwise false. */
>> +bool
>> +rs6000_pair_fusion::fuseable_load_p (insn_info *info)
>> +{
>> + rtx_insn *insn = info->rtl ();
>> +
>> + for (rtx note = REG_NOTES (insn); note; note = XEXP (note, 1))
>> + if (REG_NOTE_KIND (note) == REG_EQUAL
>> + || REG_NOTE_KIND (note) == REG_EQUIV)
>> + return false;
>
> It's unusual to punt on an optimisation because of a REG_EQUAL/EQUIV
> note. What's the reason for doing this? Are you trying to avoid
> fusing pairs before reload that are equivalent to a MEM (i.e. have
> a natural spill slot)? I think Alex hit a similar situation.
>
Removed the above check and addressed in the new patch sent.
>> +
>> + for (auto def : info->defs ())
>> + {
>> + auto set = dyn_cast<set_info *> (def);
>> + if (set && set->has_any_uses ())
>> + {
>> + for (auto use : set->all_uses())
>
> Nit: has_any_uses isn't necessary: the inner loop will simply do nothing
> in that case. Also, we can/should restrict the scan to non-debug uses.
>
> This can then be:
>
> for (auto def : info->defs ())
> if (auto set = dyn_cast<set_info *> (def))
> for (auto use : set->nondebug_insn_uses())
>
>> + {
>> + if (use->insn ()->is_artificial ())
>> + return false;
>> +
>> + insn_info *info = use->insn ();
>> +
>> + if (info
>> + && info->rtl ()
>
> This test shouldn't be necessary.
>
>> + && info->is_real ())
>> + {
>> + rtx_insn *rtl_insn = info->rtl ();
>> + rtx set = single_set (rtl_insn);
>> +
>> + if (set == NULL_RTX)
>> + return false;
>> +
>> + rtx op0 = SET_SRC (set);
>> + if (GET_CODE (op0) != UNSPEC)
>> + return false;
>
> What's the motivation for rejecting unspecs? It's unsual to treat
> all unspecs as a distinct group.
>
> Also, using single_set means that the function still lets through
> parallels of two sets in which the sources are unspecs. Is that
> intentional?
>
> The reasons behind things like the REG_EQUAL/EQUIV and UNSPEC decisions
> need to be described in comments, so that other people coming to this
> code later can understand the motivation. The same thing applies to
> other decisions in the patch.
>
Addressed description in code in new patch sent.
>> + }
>> + }
>> + }
>> + }
>> + return true;
>> +}
>> [...]
>> diff --git a/gcc/pair-fusion.cc b/gcc/pair-fusion.cc
>> index 9f897ac04e2..2dbe9f854ef 100644
>> --- a/gcc/pair-fusion.cc
>> +++ b/gcc/pair-fusion.cc
>> @@ -312,7 +312,7 @@ static int
>> encode_lfs (lfs_fields fields)
>> {
>> int size_log2 = exact_log2 (fields.size);
>> - gcc_checking_assert (size_log2 >= 2 && size_log2 <= 4);
>> + gcc_checking_assert (size_log2 >= 2 && size_log2 <= 6);
>> return ((int)fields.load_p << 3)
>> | ((int)fields.fpsimd_p << 2)
>> | (size_log2 - 2);
>
> The point of the assert is to make sure that "size_log2 - 2"
> fits within 2 bits, and so does not interfere with the fpsimd_p
> and load_p parts of the encoding. If rs6000 needs size_log2 == 6,
> the shift amounts should be increased by 1 to compensate.
>
> If we do bump the shifts by 1, the new range can be:
>
> gcc_checking_assert (size_log2 >= 2 && size_log2 <= 9);
>
Addressed in the new patch sent.
>> [...]
>> + // Given insn_info pair I1 and I2, return true if offsets are in order.
>> + virtual bool should_handle_unordered_insns (rtl_ssa::insn_info *i1,
>> + rtl_ssa::insn_info *i2) = 0;
>> +
>
> This name seems a bit misleading. The function is used in:
>
> @@ -2401,6 +2405,9 @@ pair_fusion_bb_info::try_fuse_pair (bool load_p,
> unsigned access_size,
> reversed = true;
> }
>
> + if (!m_pass->should_handle_unordered_insns (i1, i2))
> + return false;
> +
> rtx cand_mems[2];
> rtx reg_ops[2];
> rtx pats[2];
>
> and so it acts as a general opt-out. The insns aren't known to be unordered.
>
> It looks like the rs6000 override requires the original insns to be
> in offset order. Could you say why that's necessary? (Both in email
> and as a comment in the code.)
>
> If we do need a general opt-out like this, it should probably go at the
> very start of try_fuse_pair, before even the dump message. (Alternatively,
> it could go after the dump message, but then the "return false" would need
> a dump message of its own to explain the failure.)
>
Addressed comments in the code and move the check at the start of the
try_fuse_pair
functionn in new patch sent.
> Thanks,
> Richard
Thanks & Regards
Ajit
