Patch pending review

[Franz Sirl]

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.

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

Franz.