Re: Patch pending review

[Geoff Keating <geoffk at geoffk dot org>]

> From: Franz Sirl <Franz.Sirl-kernel@lauterbach.com>
> Date: Tue, 12 Dec 2000 21:51:06 +0100
> Cc: gcc-patches@gcc.gnu.org
> 
> On Tuesday 12 December 2000 20:46, Geoff Keating wrote:
> > Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
> > > Hi,
> > >
> > > this patch
> > >
> > > <http://gcc.gnu.org/ml/gcc-patches/2000-11/msg00202.html>
> > >
> > > hasn't been reviewed yet. It works fine and enables successful RTL
> > > checking bootstraps on powerpc-linux-gnu.
> >
> > Thank you!
> >
> > I was trying to work out why my patch, which changed it to:
> >
> > ;; Length (in bytes).
> > (define_attr "length" ""
> >   (if_then_else (eq_attr "type" "branch")
> >               (if_then_else (and (ge (minus (pc) (match_dup 0))
> >                                       (const_int -32768))
> >                                  (lt (minus (pc) (match_dup 0))
> >                                       (const_int 32764)))
> >                              (const_int 4)
> >                              (const_int 8))
> >
> > wasn't working.
> >
> > I believe the correct change is to make it
> >
> > ;; Length (in bytes).
> > ; According to insn_current_reference_address, '(pc)' in the following
> > means: ;   for a forward branch, the reference address is the end address
> > of ;   the branch as known from the previous branch shortening pass,
> > ;   minus a value to account for possible size increase due to
> > ;   alignment.  For a backward branch, it is the start address of the
> > ;   branch as known from the current pass, plus a value to account for
> > ;   possible size increase due to alignment.
> > ; so for forward branches, we need to subtract the size of the branch
> > ; (which will be 4 if the branch is being shortened) from (pc), to get
> > ; the start of the branch from which the branch offset will be
> > ; relative.  This is accomplished by using 32764 rather than 32768 below.
> > (define_attr "length" ""
> >   (if_then_else (eq_attr "type" "branch")
> >               (if_then_else (and (ge (minus (match_dup 0) (pc))
> >                                       (const_int -32768))
> >                                  (lt (minus (match_dup 0) (pc))
> >                                       (const_int 32764)))
> >                              (const_int 4)
> >                              (const_int 8))
> >
> > and drop the generic change.  (The generic change would break many
> > other ports.)  I'll test this and see how it goes.
> 
> Well, I guess it still may fail and I strongly believe 
> insn_current_reference_address() should be changed too. The code in question 
> as I understand it
> 
>    if (INSN_SHUID (branch) < INSN_SHUID (dest))
>     {
>       /* Forward branch. */
>       return (insn_last_address + insn_lengths[seq_uid]
>               - align_fuzz (seq, dest, length_unit_log, ~0));
>     }
>   else
>     {
>       /* Backward branch. */
>       return (insn_current_address
>               + align_fuzz (dest, seq, length_unit_log, ~0));
>     }
> 
> tries to provide a threshold so we don't enter an endless loop trying to 
> shorten the branches. But this will only work as expected if 
> insn_last_address < insn_current_address. But the branch shortening can 
> happily end up with insn_last_address >= insn_current_address and thus the 
> branch maybe miscalculated depending on the compiled code, cause the 
> calculated forward branch distance is too small then.

I think that the current code is correct.

The first thing you should know is that for labels after the current
insn, the address of the label hasn't been updated since the last
pass.  So it's correct to use insn_last_address for such computations.
By comparison, labels before the current insn _have_ been updated, so
it's OK to use insn_current_address.

The next thing to know is that it seems to assume that forward
branches are relative to the end of the branch, but backward branches
are relative to the start of the branch.  I guess this is no worse
than the alternatives.

> If my original solution seems to strict, I would suggest this variation, 
> which should accomplish the intention:
> 
>    if (INSN_SHUID (branch) < INSN_SHUID (dest))
>     {
>       /* Forward branch. */
>       return (insn_current_address - insn_default_length (branch)
>               - align_fuzz (seq, dest, length_unit_log, ~0));
>     }
>   else
>     {
>       /* Backward branch. */
>       return (insn_current_address
>               + align_fuzz (dest, seq, length_unit_log, ~0));
>     }
> 
> This should provide the threshold in a reliable manner due to the
> subtraction of insn_default_length(branch).

This is excessively conservative.

-- 
- Geoffrey Keating <geoffk@geoffk.org>