Re: Ping: RFA: add lock_length attribute to break branch-shortening cycles

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Oct 16, 2012 at 9:35 PM, Joern Rennecke
<joern.rennecke@embecosm.com> wrote:
> Quoting Richard Sandiford <rdsandiford@googlemail.com>:
>
>> Joern Rennecke <joern.rennecke@embecosm.com> writes:
>>>
>>> 2012-10-04  Joern Rennecke  <joern.rennecke@embecosm.com>
>>>
>>>          * final.c (get_attr_length_1): Use direct recursion rather than
>>>          calling get_attr_length.
>>>          (get_attr_lock_length): New function.
>>>          (INSN_VARIABLE_LENGTH_P): Define.
>>>          (shorten_branches): Take HAVE_ATTR_lock_length into account.
>>>          Don't overwrite non-delay slot insn lengths with the lengths of
>>>          delay slot insns with same uid.
>>>          * genattrtab.c (lock_length_str): New variable.
>>>          (make_length_attrs): New parameter base.
>>>          (main): Initialize lock_length_str.
>>>          Generate lock_lengths attributes.
>>>          * genattr.c (gen_attr): Emit declarations for lock_length
>>> attribute
>>>         related functions.
>>>         * doc/md.texi (node Insn Lengths): Document lock_length
>>> attribute.
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg00383.html
>>
>>
>> Sorry, this is really just repeating Richard B's comments, but I still
>> don't understand why we need two attributes.  Why can't shorten_branches
>> work with the existing length and simply make sure that the length doesn't
>> decrease from one iteration to the next?  That seems to be how you
>> implement
>> CASE_VECTOR_SHORTEN_MODE.  It also means that we can continue to use the
>> pessimistic algorithm for -O0.
>
>
> That does not really work for ARCompact, non-branch instructions are
> lengthened to align or un-align branch instructions.  Failure to
> correct the size of such instruction downwards when indicated messes
> not only up the directly affected branch, but also gets the alignment
> wrong downstream, and can even cause branches go out of range (blindsiding
> branch shortening) when there are interactions with explicit alignments
> for things like loops.
> Unless you think it's OK to poke the contents of the insn_length array from
> ADJUST_INSN_LENGTH.  That should work, but it'd require an extra parameter
> when you want to hookize this macro.
>
>
>> You said in your reply that one of the reasons was to avoid
>> "interesting" interactions with ADJUST_INSN_LENGTH.  But the
>> previous minimum length would be applied after ADJUST_INSN_LENGTH,
>> so I'm not sure why it's a factor.
>
>
> Well, for starters, the ARCompact ADJUST_INSN_LENGTH uses
> get_attr_lock_length when processing delayed-branch SEQUENCEs.
> And then there's the short/long instruction opcode distinction (function /
> attribute verify_short), which depends on alignment, and influences
> instruction length and conditional execution calculation.
> Without exact alignment information, branch scheduling becomes a crapshot.
>
> You might think I could subsume the length effect of the upsized
> instructions
> in the affected branch, but that doesn't work because the exact length is
> needed both for ADJUST_INSN_LENGTH, and in order to output the correct
> branch / jump output template.
>
>
>
>> If lock_length is just an optimisation on top of that, then maybe
>> it would help to split it out.
>
>
> Well, to the extent that branch shortening and scheduling are just
> optimizations, having the lock_length attribute is just an optimization.
>
> Well, we could split it anyway, and give ports without the need for
> multiple length attributes the benefit of the optimistic algorithm.
>
> I have attached a patch that implements this.

Looks reasonable to me, though I'm not familiar enough with the code
to approve it.

I'd strongly suggest to try harder to make things work for you without
the new attribute even though I wasn't really able to follow your reasoning
on why that wouldn't work.  It may be easier to motivate this change
once the port is in without that attribute so one can actually look at
the machine description and port details.

Richard.

> I first meant to test it for sh-elf, alas, PR54938 currently makes this
> impossible.
> So I used arm-eabi instead.  regression tests look OK, but libgcc.a and
> libc.a (newlib) are unchanged in size, while libdtfc++.a increaes by twelve
> bytes; more specifically, the size increase happens for the object file
> debug.o, which goes from 11028 to 11040 bytes.
> Likewise, cris-elf and mipsel-elf show a small increase in size of debug.o,
> with the rest of libstdc++.a unchanged in size.
>
> Generating and comparing the arm intermediate files debug.s shows a
> suspicious
> incidence of pathnames.  Using a longer build directory pathname makes no
> difference, though.
> OTOH, using a longer source directory pathname does.  So that leaves...
> makes no difference for building these libraries.
> It should stop endless looping in shorten_branches, though.  Albeit that
> is only releant for ports that do have some negative shorten_branch
> feedback.