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. >
We have used the above check because of some SPEC benchmarks failing with with MEM pairs having REG_EQUAL/EQUIV notes. By adding the checks the benchmarks passes and also it improves the performance. This checks were added during initial implementation of pair fusion pass. I will investigate further if this check is still required or not. Sorry for the inconvenience caused. >> + >> + 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()) > Sure. I will change as above. >> + { >> + if (use->insn ()->is_artificial ()) >> + return false; >> + >> + insn_info *info = use->insn (); >> + >> + if (info >> + && info->rtl () > > This test shouldn't be necessary. > Sure I will remove this check. >> + && 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. > Adjacent load pair fusion with 256 bit OOmode is seen and valid with use of load in UNSPEC. Thats why this check is added. >> + } >> + } >> + } >> + } >> + 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); > Sure I will make this change. >> [...] >> + // 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.) > Yes rs6000 requires the original load insns to be in offset order. Some regression tests like vect-outer-4f fails if we do load pair fusion with load offsets are not in offset order as this breaks lxvp semantics. > 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.) > Sure. > Thanks, > Richard Thanks & Regards Ajit