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

[Richard Sandiford <richard.sandiford@linaro.org>]

> 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,