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

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

Hi Richard,

I do not see the said patch applied in ToT yet. When do you expect it
to be available in ToT?

- Thanks and regards,
  Sameera D.

On 30 March 2018 at 17:01, Sameera Deshpande
<sameera.deshpande@linaro.org> wrote:
> Hi Richard,
>
> The testcase is working with the patch you suggested, thanks for
> pointing that out.
>
> On 30 March 2018 at 16:54, Sameera Deshpande
> <sameera.deshpande@linaro.org> wrote:
>> On 30 March 2018 at 16:39, Richard Sandiford
>> <richard.sandiford@linaro.org> wrote:
>>>> Hi Sudakshina,
>>>>
>>>> Thanks for pointing that out. Updated the conditions for attribute
>>>> length to take care of boundary conditions for offset range.
>>>>
>>>> Please find attached the updated patch.
>>>>
>>>> I have tested it for gcc testsuite and the failing testcase. Ok for trunk?
>>>>
>>>> On 22 March 2018 at 19:06, Sudakshina Das <sudi.das@arm.com> wrote:
>>>>> 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
>>>
>>> [...]
>>>
>>>> @@ -466,14 +459,9 @@
>>>>    [(set_attr "type" "branch")
>>>>     (set (attr "length")
>>>>       (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -1048576))
>>>> -                        (lt (minus (match_dup 2) (pc)) (const_int 1048572)))
>>>> +                        (le (minus (match_dup 2) (pc)) (const_int 1048572)))
>>>>                     (const_int 4)
>>>> -                   (const_int 8)))
>>>
>>> Sorry for not replying earlier, but I think the use of "lt" rather than
>>> "le" in the current length attribute is deliberate.  Distances measured
>>> from (pc) in "length" are a bit special in that backward distances are
>>> measured from the start of the instruction and forward distances are
>>> measured from the end of the instruction:
>>>
>>>       /* The address of the current insn.  We implement this actually as the
>>>          address of the current insn for backward branches, but the last
>>>          address of the next insn for forward branches, and both with
>>>          adjustments that account for the worst-case possible stretching of
>>>          intervening alignments between this insn and its destination.  */
>>>
>>> This avoids the chicken-and-egg situation of the length of the branch
>>> depending on the forward distance and the forward distance depending
>>> on the length of the branch.
>>>
>>> In contrast, this code:
>>>
>>>> @@ -712,7 +695,11 @@
>>>>    {
>>>>      if (get_attr_length (insn) == 8)
>>>>        {
>>>> -     if (get_attr_far_branch (insn) == 1)
>>>> +     long long int offset;
>>>> +     offset = INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
>>>> +               - INSN_ADDRESSES (INSN_UID (insn));
>>>> +
>>>> +     if (offset < -1048576 || offset > 1048572)
>>>>         return aarch64_gen_far_branch (operands, 2, "Ltb",
>>>>                                        "<inv_tb>\\t%<w>0, %1, ");
>>>>       else
>>>
>>> is reading the final computed addresses, so the code is right to use
>>> the real instruction range.  (FWIW I agree with Kyrill that using
>>> IN_RANGE with hex constants would be clearer.)
>>>
>>> That said... a possible problem comes from situations like:
>>>
>>>    address length insn
>>>    ......c      8 A
>>>                   ..align to 8 bytes...
>>>    ......8        B
>>>    ......c      4 C
>>>                   ..align to 16 bytes...
>>>    ......0        D, branch to B
>>>
>>> when D is at the maximum extent of the branch range and when GCC's length
>>> for A is only a conservative estimate.  If the length of A turns out to
>>> be 4 rather than 8 at assembly time, the alignment to 8 bytes will be
>>> a no-op and the address of B and C will be 8 less than we calculated.
>>> But the alignment to 16 bytes will then insert 8 bytes of padding rather
>>> than none, and the address of D won't change.  The branch will then be
>>> out of range even though the addresses calculated by GCC showed it as
>>> being in range.  insn_current_reference_address accounts for this, and
>>> so copes correctly with conservative lengths.
>>>
>>> The length can legitimately be conservative for things like asm
>>> statements, so I guess using the far_branch attribute is best
>>> after all.  Sorry for the wrong turn.
>>>
>>> On the face of it (without access to the testcase), the only problem
>>> with using far_branch in the output template is that insn_last_address
>>> is not set, but needs to be for insn_current_reference_address to do
>>> the right thing for forward branches.  Does the patch below work for
>>> your testcase?
>>>
>>> (As the documentation you mentioned in the original covering message
>>> says, it wouldn't be correct to use far_branch in anything other
>>> than final, since only the "length" attribute can respond to changes
>>> in addresses while the lengths are being calculated.  But using it
>>> in final should be fine.)
>>>
>>> Thanks,
>>> Richard
>>>
>>>
>>> 2018-03-31  Richard Sandiford  <richard.sandiford@linaro.org>
>>>
>>> gcc/
>>>         * final.c (final_1): Set insn_last_address to the same value as
>>>         insn_current_address.
>>>
>>> Index: gcc/final.c
>>> ===================================================================
>>> --- gcc/final.c 2018-03-30 11:54:02.058881968 +0100
>>> +++ gcc/final.c 2018-03-30 11:54:02.228869184 +0100
>>> @@ -2081,6 +2081,9 @@ final_1 (rtx_insn *first, FILE *file, in
>>>             }
>>>           else
>>>             insn_current_address = INSN_ADDRESSES (INSN_UID (insn));
>>> +         /* final can be seen as an iteration of shorten_branches that
>>> +            does nothing (since a fixed point has already been reached).  */
>>> +         insn_last_address = insn_current_address;
>>>         }
>>>
>>>        dump_basic_block_info (file, insn, start_to_bb, end_to_bb,
>>
>> Hi Richard,
>>
>> Thanks for the explanation. The problem was indeed because correct
>> insn_last_address was not set properly, because of which the attribute
>> FAR_BRANCH didn't work appropriately. However, I am not sure if that
>> was the only problem. Will check the testcase with the ToT sans my
>> changes, and will revert.
>>
>> --
>> - Thanks and regards,
>>   Sameera D.
>
>
>
> --
> - Thanks and regards,
>   Sameera D.



-- 
- Thanks and regards,
  Sameera D.