This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Ping / update: Re: RFA: hookize ADJUST_INSN_LENGTH


Quoting Richard Sandiford <rdsandiford@googlemail.com>:

[responding because you kept me on cc:]

Joern Rennecke <joern.rennecke@embecosm.com> writes:
This uses the same interface as my previous patch:
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00473.html ,
but I refined the algorithm for the get_insn_variants
mechanism to work properly with the reworked ARC port -
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01891.html -
the only user so far, and added some documentation.

Bootstrapped in r193731 on i686-pc-linux-gnu .

In http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00029.html the conversation was roughly:

   > Here's a very general (and rather complicated) interface,
   I wasn't after something as complicated as that.  I just
   wanted to model alignment.

and it looks like this patch implements the very general and rather
complicated interface. :-)

For the port writer, the complexities exposed are proportional to the complexities of the port that (s)he wants to describe.

 Obviously you're completely free to do that,
but it means that any comments from me are going to be same (and as
unhelpful and unproductive) as last time.  So I still think I should
bow out of this one.

In case that seems unfair, here's a summary of my concerns:

1) As Richard B says, having "locked lengths" with the comment "care must
   be taken to avoid cycles" doesn't sound like good design.  So the
   question was: without this, why would the length be going up and down
   "arbitrarily", even though we're trying to reach a fixed point?
   And the good answer seemed to be: because the backend wants to grow
   some instructions in order to give a particular (mis)alignment to
   later ones.  There's no inherent cycle problem with that, because
   although the length of an instruction can decrease when the padding
   it previously contained is no longer needed, the instruction addresses
   themselves still only move in one direction.  We don't need a locked
   length for this case.

I've seen cycles between different alignment requirements for different instructions and branch lengths that were borderline. The new approach to stabilize on instruction variants that are available should give a saner convergence, but I was afraid there might be more complex cycles that remain. Well, I'll try to remove the lock_length logic and see what happens. But obviously I can't test with all possible code that could be compiled - I just do a test build of elf32 arc-linux-uclibc toolchain including libraries and linux kernels. It could happen that that suceeds but we find a few months later that there is something else that causes cycles.

2) Another reason the lengths could go up and down "arbitrarily" is because
   the backend wants to perform if-conversion, predication, call-it-what-
   you-will on the fly during shorten_branches, and can therefore delete
   or restore instructions relative to previous iterations.  I'm still
   not convinced that's something we should support.

3) As mentioned previously, I don't think ADJUST_LENGTH should be
   aware of ADJUST_LENGTH calls for other instructions.  The way that
   the ARC port "simulates" instructions between the insn passed in the
   previous and current calls doesn't seem right.

I have removed these aspects of the ARC port, as you can see in: http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01891.html

The new approach is to do an arc-specific if-conversion based on the ccfsm
action, and run this pass at a few strategic places, so that branch_shortening
sees these effects in the rtl.

4) The patch provides a complex interface and allows for complex decisions
   to be made, but the results of those decisions aren't stored in the rtl.
   That's not a problem for ARC because ARC keeps an on-the-side FSM
   to track these things.  I don't think we should encourage or require
   that though.

As stated above, the arc port no longer uses the FSM during branch shortening. The interface needs to be complex because the target instruction selection has to take complex circumstances into account.

FWIW, one interface that would deal with the alignment side of things is:

unsigned HOST_WIDE_INT preferred_insn_alignment (rtx insn);

which returns an instruction's preferred alignment, and which could
(if useful) be called for SEQUENCEs and/or the insns inside them.


No, that doesn't make sense, because alignment cost differentials depend on instruction length. As length changes during branch shortening, you'd get different alignment requirements, and thus infinite cycles.

    rtx longer_insn_form (rtx insn, unsigned HOST_WIDE_INT current_length,
                          unsigned HOST_WIDE_INT target_length);

which returns a pattern that would turn INSN from being CURRENT_LENGTH
to TARGET_LENGTH bytes in length, or null if no such pattern exists.

And again you would ignore the cost and alignment interactions.


shorten_branches would only use these hooks when optimising the insns
for speed rather than size (the posted patch doesn't seem to account
for that).  It could record the original pattern internally so that
it can revert to that pattern if the extra padding is no longer needed.

That isn't quite right either, because we are supposed to do do speed optimizations as long as they don't hurt size. So lengthening an insn to avoid inserting a nop in front of an alignment would be nice.

longer_insn_form could be used for label alignment as well as
instruction alignment.

There's the potential for a third hook to return the possible target
lengths for a given instruction, a bit like the one in the patch,
so that alignment could be distributed over several instructions.
I think that's an unnecessary complication at this stage though.
(I don't think the original ARC submission supported it.)

Indeed, the focus for the ARC port is on (mis)alignment in terms of an instruction address mod 4 being either 0 or 2. This can be archived by upsizing a single instruction. It would be desirable to have the ability to also do cache-line aware optimizations, e.g. when you have a bunch of long instructions, and a hot path branches to them, we'd prefer there to be no cache line boundary right after or inside the first instruction. The interface I've designed does support that, the current iteration of the code does not.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]