On 10/15/2015 11:40 AM, Kyrill Tkachov wrote:
The code that analyzes the offsets of the loads/stores doesn't try to
handle load/store-multiple insns.
These appear rather frequently in memory streaming workloads on aarch64
in the form of load-pair/store-pair instructions
i.e. ldp/stp.  In RTL, they are created by the sched_fusion pass + a
subsequent peephole and during sched2 they appear
as PARALLEL rtxes of multiple SETs to/from memory.


     * sched-int.h (struct autopref_multipass_data_): Remove offset
     field.  Add min_offset, max_offset, multi_mem_insn_p fields.
     * haifa-sched.c (analyze_set_insn_for_autopref): New function.
     (autopref_multipass_init): Use it.  Handle PARALLEL sets.
     (autopref_rank_data): New function.
     (autopref_rank_for_schedule): Use it.
     (autopref_multipass_dfa_lookahead_guard_1): Likewise.

Looks pretty reasonable to me. Ok to commit with a few changes next Wednesday unless you hear from Vlad in the meantime (I just want to give him time to look at it).

+/* Helper for autopref_multipass_init.  Given a SET insn in PAT and whether
+   we're expecting a memory WRITE or not, check that the insn is relevant to
+   the autoprefetcher modelling code.  Return true iff that is the case.
+   If it is relevant record the base register of the memory op in BASE and
+   the offset in OFFSET.  */

Comma before "record". I think I'd just use "SET" rather than "SET insn", because in my mind an insn is always more than just a (part of a) pattern.

+static bool
+analyze_set_insn_for_autopref (rtx pat, int write, rtx *base, int *offset)

bool write?

+  /* This insn is relevant for auto-prefetcher.

"the auto-prefetcher".

    if (data2->status == AUTOPREF_MULTIPASS_DATA_IRRELEVANT)
      return 0;

-  if (rtx_equal_p (data1->base, data2->base)
-      && data1->offset > data2->offset)
+  bool delay_p = rtx_equal_p (data1->base, data2->base)
+                 && autopref_rank_data (data1, data2) > 0;
+  if (delay_p)

If you want to do this you still need parentheses around the multi-line expression. I'd just keep it inside the if condition.

+  /* Is this a load/store-multiple instruction.  */
+  bool multi_mem_insn_p;

Maybe write "True if this is a ..."


Bernd

Reply via email to