[PATCH] Restrict LOOP_ALIGN to loop headers only.

[Richard Biener]

On Wed, Jul 10, 2019 at 5:52 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On July 10, 2019 2:11:17 PM GMT+02:00, Michael Matz <matz@suse.de> wrote:
> >Hi,
> >
> >On Tue, 9 Jul 2019, Richard Biener wrote:
> >
> >> >The basic block index is not a DFS index, so no, that's not a test
> >for
> >> >backedge.
> >>
> >> I think in CFG RTL mode the BB index designates the order of the BBs
> >in
> >> the object file? So this is a way to identify backwards jumps?
> >
> >Even if it means a backwards jump (and that's not always the case, the
> >insns are emitted by following the NEXT_INSN links, without a CFG, and
> >that all happens after machine-dependend reorg, and going out of cfg
> >layout might link insn together even from high index BBs to low index
> >BBs
> >(e.g. because of fall-through)), that's still not a backedge in the
> >general case.  If a heuristic is enough here it might be okay, though.
> >
> >OTOH, as here a CFG still exists, why not simply rely on a proper DFS
> >marking backedges?
>
> Because proper backedges is not what we want here, see honzas example.
>
> So I'm second-guessing why we have different LOOP_ALIGN and when it makes sense to apply.

So docs have

@defmac JUMP_ALIGN (@var{label})
The alignment (log base 2) to put in front of @var{label}, which is
a common destination of jumps and has no fallthru incoming edge.

...

@defmac LOOP_ALIGN (@var{label})
The alignment (log base 2) to put in front of @var{label} that heads
a frequently executed basic block (usually the header of a loop).

so I would expect the alignment pass to have

      if ( (branch_count > count_threshold
              || (bb->count > bb->prev_bb->count.apply_scale (10, 1)
                  && (bb->prev_bb->count
                      <= ENTRY_BLOCK_PTR_FOR_FN (cfun)
                           ->count.apply_scale (1, 2)))))
        {
          align_flags alignment = has_fallthru ? JUMP_ALIGN (label) :
LOOP_ALIGN (label);
          if (dump_file)
            fprintf (dump_file, "  jump alignment added.\n");
          max_alignment = align_flags::max (max_alignment, alignment);
        }

instead of the two different conditions?  Or the JUMP_ALIGN case
computing "common destination" instead of "frequently executed"
somehow but I think it makes sense that those are actually the same
here (frequently executed).  It oddly looks at prev_bb and is not
guarded with optimize_bb_for_speed_p at all so this all is
full of heuristics and changing anything here just based on x86
measurements is surely going to cause issues for targets more
sensitive to (mis-)alignment.

Richard.

> Richard.
>
> >
> >Ciao,
> >Michael.
>