Re: Patch pending review

[Franz Sirl <Franz dot Sirl-kernel at lauterbach dot com>]

On Tuesday 12 December 2000 22:33, Geoff Keating wrote:
> > 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:
> > 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.

Hmm, this contradicts my experience during debugging IIRC, what happens if 
the forward branch distance gets longer during this pass?

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

Hmm.

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

I don't think so, during the last pass the pessimization should be minimal. 
Well, lets see what your testing reveals.

Franz.