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

[Sameera Deshpande <sameera dot deshpande at linaro dot org>]

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.

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



-- 
- Thanks and regards,
  Sameera D.