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....@arm.com> wrote:
> On 15/03/18 15:27, Sameera Deshpande wrote:
>>
>> Ping!
>>
>> On 28 February 2018 at 16:18, Sameera Deshpande
>> <sameera.deshpa...@linaro.org> wrote:
>>>
>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>>> <ramana....@googlemail.com> wrote:
>>>>
>>>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>>>> <sameera.deshpa...@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.deshpa...@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.
