Re: [Aarch64] Fix conditional branches with target far away.

[Sudakshina Das <sudi dot das at arm dot com>]

Hi Sameera

On 22/03/18 02:07, Sameera Deshpande wrote:
> Hi Sudakshina,
>
> As per the ARMv8 ARM, for the offset range (-1048576 ,1048572), the
> far branch instruction offset is inclusive of both the offsets. Hence,
> I am using <=||=> and not <||>= as it was in previous implementation.

I have to admit earlier I was only looking at the patch mechanically and 
found a difference with the previous implementation in offset 
comparison. After you pointed out, I looked up the ARMv8 ARM and I have 
a couple of doubts:

1. My understanding is that any offset in [-1048576 ,1048572] both 
inclusive qualifies as an 'in range' offset. However, the code for both 
attribute length and far_branch has been using [-1048576 ,1048572), that 
is, ( >= && < ). If the far_branch was incorrectly calculated, then 
maybe the length calculations with similar magic numbers should also be 
corrected? Of course, I am not an expert in this and maybe this was a 
conscience decision so I would ask Ramana to maybe clarify if he remembers.

2. Now to come back to your patch, if my understanding is correct, I 
think a far_branch would be anything outside of this range, that is,
(offset < -1048576 || offset > 1048572), anything that can not be 
represented in the 21-bit range.

Thanks
Sudi

> On 16 March 2018 at 00:51, Sudakshina Das <sudi.das@arm.com> wrote:
> > On 15/03/18 15:27, Sameera Deshpande wrote:
> > > Ping!
> > >
> > > On 28 February 2018 at 16:18, Sameera Deshpande
> > > <sameera.deshpande@linaro.org> wrote:
> > > > On 27 February 2018 at 18:25, Ramana Radhakrishnan
> > > > <ramana.gcc@googlemail.com> wrote:
> > > > > On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
> > > > > <sameera.deshpande@linaro.org> wrote:
> > > > > > Hi!
> > > > > >
> > > > > > Please find attached the patch to fix bug in branches with offsets over
> > > > > > 1MiB.
> > > > > > There has been an attempt to fix this issue in commit
> > > > > > 050af05b9761f1979f11c151519e7244d5becd7c
> > > > > >
> > > > > > However, the far_branch attribute defined in above patch used
> > > > > > insn_length - which computes incorrect offset. Hence, eliminated the
> > > > > > attribute completely, and computed the offset from insn_addresses
> > > > > > instead.
> > > > > >
> > > > > > Ok for trunk?
> > > > > >
> > > > > > gcc/Changelog
> > > > > >
> > > > > > 2018-02-13 Sameera Deshpande <sameera.deshpande@linaro.org>
> > > > > >           * config/aarch64/aarch64.md (far_branch): Remove attribute.
> > > > > > Eliminate
> > > > > >           all the dependencies on the attribute from RTL patterns.
> > > > >
> > > > > I'm not a maintainer but this looks good to me modulo notes about how
> > > > > this was tested. What would be nice is a testcase for the testsuite as
> > > > > well as ensuring that the patch has been bootstrapped and regression
> > > > > tested. AFAIR, the original patch was put in because match.pd failed
> > > > > when bootstrap in another context.
> > > > >
> > > > >
> > > > > regards
> > > > > Ramana
> > > > >
> > > > > > --
> > > > > > - Thanks and regards,
> > > > > >     Sameera D.
> > > >
> > > >
> > > > The patch is tested with GCC testsuite and bootstrapping successfully.
> > > > Also tested for spec benchmark.
> >
> > I am not a maintainer either. I noticed that the range check you do for
> > the offset has a (<= || >=). The "far_branch" however did (< || >=) for a
> > positive value. Was that also part of the incorrect offset calculation?
> >
> > @@ -692,7 +675,11 @@
> >      {
> >        if (get_attr_length (insn) =3D=3D 8)
> >          {
> > -       if (get_attr_far_branch (insn) =3D=3D 1)
> > +       long long int offset;
> > +       offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
> > +                 - INSN_ADDRESSES (INSN_UID (insn));
> > +
> > +       if (offset <=3D -1048576 || offset >=3D 1048572)
> >             return aarch64_gen_far_branch (operands, 2, "Ltb",
> >                                            "<inv_tb>\\t%<w>0, %1, ");
> >           else
> > @@ -709,12 +696,7 @@
> >           (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
> > -32768))
> >                              (lt (minus (match_dup 2) (pc)) (const_int
> > 32764)))
> >                         (const_int 4)
> > -                     (const_int 8)))
> > -   (set (attr "far_branch")
> > -       (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
> > -1048576))
> > -                          (lt (minus (match_dup 2) (pc)) (const_int
> > 1048572)))
> > -                     (const_int 0)
> > -                     (const_int 1)))]
> > +                     (const_int 8)))]
> >
> >    )
> >
> > Thanks
> > Sudi
> >
> > > > --
> > > > - Thanks and regards,
> > > >     Sameera D.