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

[Sudakshina Das <sudi.das@arm.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.
> 
> 
>