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.
