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.
