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.deshpa...@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.deshpa...@linaro.org> wrote:
>> On 30 March 2018 at 16:39, Richard Sandiford
>> <richard.sandif...@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....@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.sandif...@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.
