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

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

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.