This is the mail archive of the 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

[responding because you kept me on cc:]

Joern Rennecke <> writes:
> This uses the same interface as my previous patch:
> ,
> but I refined the algorithm for the get_insn_variants
> mechanism to work properly with the reworked ARC port -
> -
> the only user so far, and added some documentation.
> Bootstrapped in r193731 on i686-pc-linux-gnu .

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. :-)  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.

   That kind of alignment optimisation sounds like a useful thing to have,
   but it should be modelled explicitly.  The new patch does model it
   explicitly but still keeps the "locked length: to avoid cycles" thing
   as well.

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.

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.

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.

    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.

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.
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.)

But like I say, that's all just for the record.


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