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

[Sameera Deshpande]

On Mon 9 Apr, 2018, 2:06 PM Sameera Deshpande, 
wrote:

> 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
>  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
> >  wrote:
> >> On 30 March 2018 at 16:39, Richard Sandiford
> >>  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  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",
> "\\t%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...
> >>>..8B
> >>>..c  4 C
> >>>   ..align to 16 bytes...
> >>>..0D, branch to B
> >>>
> >>> when D is at the maximum extent of 

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

[Sameera Deshpande]

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
 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
>  wrote:
>> On 30 March 2018 at 16:39, Richard Sandiford
>>  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  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",
"\\t%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...
>>>..8B
>>>..c  4 C
>>>   ..align to 16 bytes...
>>>..0D, 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 

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

[Sameera Deshpande]

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
 wrote:
> On 30 March 2018 at 16:39, Richard Sandiford
>  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  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",
>>>"\\t%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...
>>..8B
>>..c  4 C
>>   ..align to 16 bytes...
>>..0D, 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.  

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

[Sameera Deshpande]

On 30 March 2018 at 16:39, Richard Sandiford
 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  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",
>>"\\t%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...
>..8B
>..c  4 C
>   ..align to 16 bytes...
>..0D, 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 

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

[Richard Sandiford]

> 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  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",
>"\\t%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...
   ..8B
   ..c  4 C
  ..align to 16 bytes...
   ..0D, 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 

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

[Kyrill Tkachov]

Hi Sameera,

On 29/03/18 11:44, Sameera Deshpande 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  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
>
>
>>
>> On 16 March 2018 at 00:51, Sudakshina Das  wrote:
>>>
>>> On 15/03/18 15:27, Sameera Deshpande wrote:


 Ping!

 On 28 February 2018 at 16:18, Sameera Deshpande
  wrote:
>
>
> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>  wrote:
>>
>>
>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>>  wrote:
>>>
>>>
>>> Hi!
>>>
>>> Please find attached the patch to fix bug in branches with offsets
>>> over
>>> 1MiB.
>>> There has been an attempt to fix this issue in commit
>>> 050af05b9761f1979f11c151519e7244d5becd7c
>>>
>>> However, the far_branch attribute defined in above patch used
>>> insn_length - which computes incorrect offset. Hence, eliminated the
>>> attribute completely, and computed the offset from insn_addresses
>>> instead.
>>>
>>> Ok for trunk?
>>>
>>> gcc/Changelog
>>>
>>> 2018-02-13 Sameera Deshpande 
>>>   * config/aarch64/aarch64.md (far_branch): Remove attribute.
>>> Eliminate
>>>   all the dependencies on the attribute from RTL patterns.
>>>


I'd list all the patterns affected by the removal. There's not many of them
and it makes checking the impact of the patch simpler.



>>
>> I'm not a maintainer but this looks good to me modulo notes about how
>> this was tested. What would be nice is a testcase for the testsuite as
>> well as ensuring that the patch has been bootstrapped and regression
>> tested. AFAIR, the original patch was put in because match.pd failed
>> when bootstrap in another context.
>>
>>
>> regards
>> Ramana
>>
>>> --
>>> - Thanks and regards,
>>> Sameera D.
>
>
>
> The patch is tested with GCC testsuite and bootstrapping successfully.
> Also tested for spec benchmark.
>
>>>
>>> I am not a maintainer either. I noticed that the range check you do for
>>> the offset has a (<= || >=). The "far_branch" however did (< || >=) for a
>>> positive value. Was that also part of the incorrect offset calculation?
>>>




I'm not a maintainer, but this looks like a good improvement to me, and is more 
readable to boot!
Getting some kind of reduced testcase (perhaps with the creduce tool, or 
derived from base principles)
would be very valuable, but I guess it shouldn't be a blocker for this patch.
I've got a couple of nits inline.

Index: gcc/config/aarch64/aarch64.md
===
--- gcc/config/aarch64/aarch64.md   (revision 258946)
+++ gcc/config/aarch64/aarch64.md   (working copy)
@@ -264,13 +264,6 @@
(const_string "no")
] (const_string "yes")))
 
-;; Attribute that specifies whether we are dealing with a branch to a

-;; label that is far away, i.e. further away than the maximum/minimum
-;; representable in a signed 21-bits number.
-;; 0 :=: no
-;; 1 :=: yes
-(define_attr "far_branch" "" (const_int 0))
-
 ;; Strictly for compatibility with AArch32 in pipeline models, since AArch64 
has
 ;; no predicated insns.
 (define_attr 

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

[Sameera Deshpande]

Hi Sudakshina,

That testcase cannot be addwd as of now, as it needs approval from client.

On Thu 29 Mar, 2018, 9:01 PM Sudakshina Das,  wrote:

> Hi Sameera
>
> On 29/03/18 11:44, Sameera Deshpande 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?
>
> Thank you so much for fixing the length as well along with you patch.
> You mention a failing testcase? Maybe it would be helpful to add that
> to the patch for the gcc testsuite.
>
> Sudi
>
> >
> > On 22 March 2018 at 19:06, Sudakshina Das  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
> >>
> >>
> >>>
> >>> On 16 March 2018 at 00:51, Sudakshina Das  wrote:
> 
>  On 15/03/18 15:27, Sameera Deshpande wrote:
> >
> >
> > Ping!
> >
> > On 28 February 2018 at 16:18, Sameera Deshpande
> >  wrote:
> >>
> >>
> >> On 27 February 2018 at 18:25, Ramana Radhakrishnan
> >>  wrote:
> >>>
> >>>
> >>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
> >>>  wrote:
> 
> 
>  Hi!
> 
>  Please find attached the patch to fix bug in branches with offsets
>  over
>  1MiB.
>  There has been an attempt to fix this issue in commit
>  050af05b9761f1979f11c151519e7244d5becd7c
> 
>  However, the far_branch attribute defined in above patch used
>  insn_length - which computes incorrect offset. Hence, eliminated
> the
>  attribute completely, and computed the offset from insn_addresses
>  instead.
> 
>  Ok for trunk?
> 
>  gcc/Changelog
> 
>  2018-02-13 Sameera Deshpande 
> * config/aarch64/aarch64.md (far_branch): Remove
> attribute.
>  Eliminate
> all the dependencies on the attribute from RTL
> patterns.
> 
> >>>
> >>> I'm not a maintainer but this looks good to me modulo notes about
> how
> >>> this was tested. What would be nice is a testcase for the
> testsuite as
> >>> well as ensuring that the patch has been bootstrapped and
> regression
> >>> tested. AFAIR, the original patch was put in because match.pd
> failed
> >>> when bootstrap in another context.
> >>>
> >>>
> >>> regards
> >>> Ramana
> >>>
>  --
>  - Thanks and regards,
>   Sameera D.
> >>
> >>
> >>
> >> The patch is tested with GCC testsuite and bootstrapping
> successfully.
> >> Also tested for spec benchmark.
> >>
> 
>  I am not a maintainer either. I noticed that the range check you do
> for
>  the offset has a (<= || >=). The "far_branch" however did (< || >=)
> for a
>  positive value. Was that also part of the incorrect offset
> calculation?
> 
>  @@ -692,7 +675,11 @@
>    {
>  if (get_attr_length (insn) =3D=3D 8)
>    {
>  -   if (get_attr_far_branch (insn) =3D=3D 1)
>  +   long long int offset;
>  +   offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
>  + - INSN_ADDRESSES (INSN_UID (insn));
>  +
>  +   if (offset <=3D -1048576 || offset >=3D 1048572)
> 

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

[Sudakshina Das]

Hi Sameera

On 29/03/18 11:44, Sameera Deshpande 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?


Thank you so much for fixing the length as well along with you patch.
You mention a failing testcase? Maybe it would be helpful to add that
to the patch for the gcc testsuite.

Sudi



On 22 March 2018 at 19:06, Sudakshina Das  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




On 16 March 2018 at 00:51, Sudakshina Das  wrote:


On 15/03/18 15:27, Sameera Deshpande wrote:



Ping!

On 28 February 2018 at 16:18, Sameera Deshpande
 wrote:



On 27 February 2018 at 18:25, Ramana Radhakrishnan
 wrote:



On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
 wrote:



Hi!

Please find attached the patch to fix bug in branches with offsets
over
1MiB.
There has been an attempt to fix this issue in commit
050af05b9761f1979f11c151519e7244d5becd7c

However, the far_branch attribute defined in above patch used
insn_length - which computes incorrect offset. Hence, eliminated the
attribute completely, and computed the offset from insn_addresses
instead.

Ok for trunk?

gcc/Changelog

2018-02-13 Sameera Deshpande 
   * config/aarch64/aarch64.md (far_branch): Remove attribute.
Eliminate
   all the dependencies on the attribute from RTL patterns.



I'm not a maintainer but this looks good to me modulo notes about how
this was tested. What would be nice is a testcase for the testsuite as
well as ensuring that the patch has been bootstrapped and regression
tested. AFAIR, the original patch was put in because match.pd failed
when bootstrap in another context.


regards
Ramana


--
- Thanks and regards,
 Sameera D.




The patch is tested with GCC testsuite and bootstrapping successfully.
Also tested for spec benchmark.



I am not a maintainer either. I noticed that the range check you do for
the offset has a (<= || >=). The "far_branch" however did (< || >=) for a
positive value. Was that also part of the incorrect offset calculation?

@@ -692,7 +675,11 @@
  {
if (get_attr_length (insn) =3D=3D 8)
  {
-   if (get_attr_far_branch (insn) =3D=3D 1)
+   long long int offset;
+   offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
+ - INSN_ADDRESSES (INSN_UID (insn));
+
+   if (offset <=3D -1048576 || offset >=3D 1048572)
 return aarch64_gen_far_branch (operands, 2, "Ltb",
"\\t%0, %1, ");
   else
@@ -709,12 +696,7 @@
   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
-32768))
  (lt (minus (match_dup 2) (pc)) (const_int
32764)))
 (const_int 4)
- (const_int 8)))
-   (set (attr "far_branch")
-   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
-1048576))
-  (lt (minus (match_dup 2) (pc)) (const_int
1048572)))
- (const_int 0)
- (const_int 1)))]
+ (const_int 8)))]

)

Thanks
Sudi


--
- Thanks and regards,
 Sameera D.

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

[Sameera Deshpande]

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  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
>
>
>>
>> On 16 March 2018 at 00:51, Sudakshina Das  wrote:
>>>
>>> On 15/03/18 15:27, Sameera Deshpande wrote:


 Ping!

 On 28 February 2018 at 16:18, Sameera Deshpande
  wrote:
>
>
> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>  wrote:
>>
>>
>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>>  wrote:
>>>
>>>
>>> Hi!
>>>
>>> Please find attached the patch to fix bug in branches with offsets
>>> over
>>> 1MiB.
>>> There has been an attempt to fix this issue in commit
>>> 050af05b9761f1979f11c151519e7244d5becd7c
>>>
>>> However, the far_branch attribute defined in above patch used
>>> insn_length - which computes incorrect offset. Hence, eliminated the
>>> attribute completely, and computed the offset from insn_addresses
>>> instead.
>>>
>>> Ok for trunk?
>>>
>>> gcc/Changelog
>>>
>>> 2018-02-13 Sameera Deshpande 
>>>   * config/aarch64/aarch64.md (far_branch): Remove attribute.
>>> Eliminate
>>>   all the dependencies on the attribute from RTL patterns.
>>>
>>
>> I'm not a maintainer but this looks good to me modulo notes about how
>> this was tested. What would be nice is a testcase for the testsuite as
>> well as ensuring that the patch has been bootstrapped and regression
>> tested. AFAIR, the original patch was put in because match.pd failed
>> when bootstrap in another context.
>>
>>
>> regards
>> Ramana
>>
>>> --
>>> - Thanks and regards,
>>> Sameera D.
>
>
>
> The patch is tested with GCC testsuite and bootstrapping successfully.
> Also tested for spec benchmark.
>
>>>
>>> I am not a maintainer either. I noticed that the range check you do for
>>> the offset has a (<= || >=). The "far_branch" however did (< || >=) for a
>>> positive value. Was that also part of the incorrect offset calculation?
>>>
>>> @@ -692,7 +675,11 @@
>>>  {
>>>if (get_attr_length (insn) =3D=3D 8)
>>>  {
>>> -   if (get_attr_far_branch (insn) =3D=3D 1)
>>> +   long long int offset;
>>> +   offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
>>> + - INSN_ADDRESSES (INSN_UID (insn));
>>> +
>>> +   if (offset <=3D -1048576 || offset >=3D 1048572)
>>> return aarch64_gen_far_branch (operands, 2, "Ltb",
>>>"\\t%0, %1, ");
>>>   else
>>> @@ -709,12 +696,7 @@
>>>   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
>>> -32768))
>>>  (lt (minus (match_dup 2) (pc)) (const_int
>>> 32764)))
>>> (const_int 4)
>>> - (const_int 8)))
>>> -   (set (attr "far_branch")
>>> -   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
>>> -1048576))
>>> -  (lt (minus (match_dup 2) (pc)) (const_int
>>> 1048572)))
>>> - (const_int 0)
>>> - (const_int 1)))]
>>> + (const_int 8)))]
>>>
>>>)
>>>
>>> Thanks
>>> Sudi
>>>
> --
> - 

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

[Sudakshina Das]

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



On 16 March 2018 at 00:51, Sudakshina Das  wrote:

On 15/03/18 15:27, Sameera Deshpande wrote:


Ping!

On 28 February 2018 at 16:18, Sameera Deshpande
 wrote:


On 27 February 2018 at 18:25, Ramana Radhakrishnan
 wrote:


On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
 wrote:


Hi!

Please find attached the patch to fix bug in branches with offsets over
1MiB.
There has been an attempt to fix this issue in commit
050af05b9761f1979f11c151519e7244d5becd7c

However, the far_branch attribute defined in above patch used
insn_length - which computes incorrect offset. Hence, eliminated the
attribute completely, and computed the offset from insn_addresses
instead.

Ok for trunk?

gcc/Changelog

2018-02-13 Sameera Deshpande 
  * config/aarch64/aarch64.md (far_branch): Remove attribute.
Eliminate
  all the dependencies on the attribute from RTL patterns.



I'm not a maintainer but this looks good to me modulo notes about how
this was tested. What would be nice is a testcase for the testsuite as
well as ensuring that the patch has been bootstrapped and regression
tested. AFAIR, the original patch was put in because match.pd failed
when bootstrap in another context.


regards
Ramana


--
- Thanks and regards,
Sameera D.



The patch is tested with GCC testsuite and bootstrapping successfully.
Also tested for spec benchmark.



I am not a maintainer either. I noticed that the range check you do for
the offset has a (<= || >=). The "far_branch" however did (< || >=) for a
positive value. Was that also part of the incorrect offset calculation?

@@ -692,7 +675,11 @@
 {
   if (get_attr_length (insn) =3D=3D 8)
 {
-   if (get_attr_far_branch (insn) =3D=3D 1)
+   long long int offset;
+   offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
+ - INSN_ADDRESSES (INSN_UID (insn));
+
+   if (offset <=3D -1048576 || offset >=3D 1048572)
return aarch64_gen_far_branch (operands, 2, "Ltb",
   "\\t%0, %1, ");
  else
@@ -709,12 +696,7 @@
  (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
-32768))
 (lt (minus (match_dup 2) (pc)) (const_int
32764)))
(const_int 4)
- (const_int 8)))
-   (set (attr "far_branch")
-   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
-1048576))
-  (lt (minus (match_dup 2) (pc)) (const_int
1048572)))
- (const_int 0)
- (const_int 1)))]
+ (const_int 8)))]

   )

Thanks
Sudi


--
- Thanks and regards,
Sameera D.

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

[Sameera Deshpande]

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.

On 16 March 2018 at 00:51, Sudakshina Das  wrote:
> On 15/03/18 15:27, Sameera Deshpande wrote:
>>
>> Ping!
>>
>> On 28 February 2018 at 16:18, Sameera Deshpande
>>  wrote:
>>>
>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>>>  wrote:

 On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
  wrote:
>
> Hi!
>
> Please find attached the patch to fix bug in branches with offsets over
> 1MiB.
> There has been an attempt to fix this issue in commit
> 050af05b9761f1979f11c151519e7244d5becd7c
>
> However, the far_branch attribute defined in above patch used
> insn_length - which computes incorrect offset. Hence, eliminated the
> attribute completely, and computed the offset from insn_addresses
> instead.
>
> Ok for trunk?
>
> gcc/Changelog
>
> 2018-02-13 Sameera Deshpande 
>  * config/aarch64/aarch64.md (far_branch): Remove attribute.
> Eliminate
>  all the dependencies on the attribute from RTL patterns.
>

 I'm not a maintainer but this looks good to me modulo notes about how
 this was tested. What would be nice is a testcase for the testsuite as
 well as ensuring that the patch has been bootstrapped and regression
 tested. AFAIR, the original patch was put in because match.pd failed
 when bootstrap in another context.


 regards
 Ramana

> --
> - Thanks and regards,
>Sameera D.
>>>
>>>
>>> The patch is tested with GCC testsuite and bootstrapping successfully.
>>> Also tested for spec benchmark.
>>>
>
> I am not a maintainer either. I noticed that the range check you do for
> the offset has a (<= || >=). The "far_branch" however did (< || >=) for a
> positive value. Was that also part of the incorrect offset calculation?
>
> @@ -692,7 +675,11 @@
> {
>   if (get_attr_length (insn) =3D=3D 8)
> {
> -   if (get_attr_far_branch (insn) =3D=3D 1)
> +   long long int offset;
> +   offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
> + - INSN_ADDRESSES (INSN_UID (insn));
> +
> +   if (offset <=3D -1048576 || offset >=3D 1048572)
>return aarch64_gen_far_branch (operands, 2, "Ltb",
>   "\\t%0, %1, ");
>  else
> @@ -709,12 +696,7 @@
>  (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
> -32768))
> (lt (minus (match_dup 2) (pc)) (const_int
> 32764)))
>(const_int 4)
> - (const_int 8)))
> -   (set (attr "far_branch")
> -   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
> -1048576))
> -  (lt (minus (match_dup 2) (pc)) (const_int
> 1048572)))
> - (const_int 0)
> - (const_int 1)))]
> + (const_int 8)))]
>
>   )
>
> Thanks
> Sudi
>
>>> --
>>> - Thanks and regards,
>>>Sameera D.
>>
>>
>>
>>
>



-- 
- Thanks and regards,
  Sameera D.

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

[Sudakshina Das]

On 15/03/18 15:27, Sameera Deshpande wrote:

Ping!

On 28 February 2018 at 16:18, Sameera Deshpande
 wrote:

On 27 February 2018 at 18:25, Ramana Radhakrishnan
 wrote:

On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
 wrote:

Hi!

Please find attached the patch to fix bug in branches with offsets over 1MiB.
There has been an attempt to fix this issue in commit
050af05b9761f1979f11c151519e7244d5becd7c

However, the far_branch attribute defined in above patch used
insn_length - which computes incorrect offset. Hence, eliminated the
attribute completely, and computed the offset from insn_addresses
instead.

Ok for trunk?

gcc/Changelog

2018-02-13 Sameera Deshpande 
 * config/aarch64/aarch64.md (far_branch): Remove attribute. Eliminate
 all the dependencies on the attribute from RTL patterns.



I'm not a maintainer but this looks good to me modulo notes about how
this was tested. What would be nice is a testcase for the testsuite as
well as ensuring that the patch has been bootstrapped and regression
tested. AFAIR, the original patch was put in because match.pd failed
when bootstrap in another context.


regards
Ramana


--
- Thanks and regards,
   Sameera D.


The patch is tested with GCC testsuite and bootstrapping successfully.
Also tested for spec benchmark.



I am not a maintainer either. I noticed that the range check you do for
the offset has a (<= || >=). The "far_branch" however did (< || >=) for 
a positive value. Was that also part of the incorrect offset calculation?


@@ -692,7 +675,11 @@
{
  if (get_attr_length (insn) =3D=3D 8)
{
-   if (get_attr_far_branch (insn) =3D=3D 1)
+   long long int offset;
+   offset =3D INSN_ADDRESSES (INSN_UID (XEXP (operands[2], 0)))
+ - INSN_ADDRESSES (INSN_UID (insn));
+
+   if (offset <=3D -1048576 || offset >=3D 1048572)
   return aarch64_gen_far_branch (operands, 2, "Ltb",
  "\\t%0, %1, ");
 else
@@ -709,12 +696,7 @@
 (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
-32768))
(lt (minus (match_dup 2) (pc)) (const_int
32764)))
   (const_int 4)
- (const_int 8)))
-   (set (attr "far_branch")
-   (if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int
-1048576))
-  (lt (minus (match_dup 2) (pc)) (const_int
1048572)))
- (const_int 0)
- (const_int 1)))]
+ (const_int 8)))]

  )

Thanks
Sudi


--
- Thanks and regards,
   Sameera D.

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

[Sameera Deshpande]

Ping!

On 28 February 2018 at 16:18, Sameera Deshpande
 wrote:
> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>  wrote:
>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>>  wrote:
>>> Hi!
>>>
>>> Please find attached the patch to fix bug in branches with offsets over 
>>> 1MiB.
>>> There has been an attempt to fix this issue in commit
>>> 050af05b9761f1979f11c151519e7244d5becd7c
>>>
>>> However, the far_branch attribute defined in above patch used
>>> insn_length - which computes incorrect offset. Hence, eliminated the
>>> attribute completely, and computed the offset from insn_addresses
>>> instead.
>>>
>>> Ok for trunk?
>>>
>>> gcc/Changelog
>>>
>>> 2018-02-13 Sameera Deshpande 
>>> * config/aarch64/aarch64.md (far_branch): Remove attribute. 
>>> Eliminate
>>> all the dependencies on the attribute from RTL patterns.
>>>
>>
>> I'm not a maintainer but this looks good to me modulo notes about how
>> this was tested. What would be nice is a testcase for the testsuite as
>> well as ensuring that the patch has been bootstrapped and regression
>> tested. AFAIR, the original patch was put in because match.pd failed
>> when bootstrap in another context.
>>
>>
>> regards
>> Ramana
>>
>>> --
>>> - Thanks and regards,
>>>   Sameera D.
>
> The patch is tested with GCC testsuite and bootstrapping successfully.
> Also tested for spec benchmark.
>
> --
> - Thanks and regards,
>   Sameera D.



-- 
- Thanks and regards,
  Sameera D.

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

[Sameera Deshpande]

On 27 February 2018 at 18:25, Ramana Radhakrishnan
 wrote:
> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>  wrote:
>> Hi!
>>
>> Please find attached the patch to fix bug in branches with offsets over 1MiB.
>> There has been an attempt to fix this issue in commit
>> 050af05b9761f1979f11c151519e7244d5becd7c
>>
>> However, the far_branch attribute defined in above patch used
>> insn_length - which computes incorrect offset. Hence, eliminated the
>> attribute completely, and computed the offset from insn_addresses
>> instead.
>>
>> Ok for trunk?
>>
>> gcc/Changelog
>>
>> 2018-02-13 Sameera Deshpande 
>> * config/aarch64/aarch64.md (far_branch): Remove attribute. Eliminate
>> all the dependencies on the attribute from RTL patterns.
>>
>
> I'm not a maintainer but this looks good to me modulo notes about how
> this was tested. What would be nice is a testcase for the testsuite as
> well as ensuring that the patch has been bootstrapped and regression
> tested. AFAIR, the original patch was put in because match.pd failed
> when bootstrap in another context.
>
>
> regards
> Ramana
>
>> --
>> - Thanks and regards,
>>   Sameera D.

The patch is tested with GCC testsuite and bootstrapping successfully.
Also tested for spec benchmark.

-- 
- Thanks and regards,
  Sameera D.

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

[Ramana Radhakrishnan]

On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
 wrote:
> Hi!
>
> Please find attached the patch to fix bug in branches with offsets over 1MiB.
> There has been an attempt to fix this issue in commit
> 050af05b9761f1979f11c151519e7244d5becd7c
>
> However, the far_branch attribute defined in above patch used
> insn_length - which computes incorrect offset. Hence, eliminated the
> attribute completely, and computed the offset from insn_addresses
> instead.
>
> Ok for trunk?
>
> gcc/Changelog
>
> 2018-02-13 Sameera Deshpande 
> * config/aarch64/aarch64.md (far_branch): Remove attribute. Eliminate
> all the dependencies on the attribute from RTL patterns.
>

I'm not a maintainer but this looks good to me modulo notes about how
this was tested. What would be nice is a testcase for the testsuite as
well as ensuring that the patch has been bootstrapped and regression
tested. AFAIR, the original patch was put in because match.pd failed
when bootstrap in another context.


regards
Ramana

> --
> - Thanks and regards,
>   Sameera D.

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

[Sameera Deshpande]

On 14 February 2018 at 14:00, Sameera Deshpande
 wrote:
> Hi!
>
> Please find attached the patch to fix bug in branches with offsets over 1MiB.
> There has been an attempt to fix this issue in commit
> 050af05b9761f1979f11c151519e7244d5becd7c
>
> However, the far_branch attribute defined in above patch used
> insn_length - which computes incorrect offset. Hence, eliminated the
> attribute completely, and computed the offset from insn_addresses
> instead.
>
> Ok for trunk?
>
> gcc/Changelog
>
> 2018-02-13 Sameera Deshpande 
> * config/aarch64/aarch64.md (far_branch): Remove attribute. Eliminate
> all the dependencies on the attribute from RTL patterns.
>
> --
> - Thanks and regards,
>   Sameera D.


Gentle reminder!

-- 
- Thanks and regards,
  Sameera D.

[Aarch64] Fix conditional branches with target far away.

[Sameera Deshpande]

Hi!

Please find attached the patch to fix bug in branches with offsets over 1MiB.
There has been an attempt to fix this issue in commit
050af05b9761f1979f11c151519e7244d5becd7c

However, the far_branch attribute defined in above patch used
insn_length - which computes incorrect offset. Hence, eliminated the
attribute completely, and computed the offset from insn_addresses
instead.

Ok for trunk?

gcc/Changelog

2018-02-13 Sameera Deshpande 
* config/aarch64/aarch64.md (far_branch): Remove attribute. Eliminate
all the dependencies on the attribute from RTL patterns.

-- 
- Thanks and regards,
  Sameera D.
Index: gcc/config/aarch64/aarch64.md
===
--- gcc/config/aarch64/aarch64.md	(revision 257620)
+++ gcc/config/aarch64/aarch64.md	(working copy)
@@ -244,13 +244,6 @@
 	(const_string "no")
 	] (const_string "yes")))
 
-;; Attribute that specifies whether we are dealing with a branch to a
-;; label that is far away, i.e. further away than the maximum/minimum
-;; representable in a signed 21-bits number.
-;; 0 :=: no
-;; 1 :=: yes
-(define_attr "far_branch" "" (const_int 0))
-
 ;; Strictly for compatibility with AArch32 in pipeline models, since AArch64 has
 ;; no predicated insns.
 (define_attr "predicated" "yes,no" (const_string "no"))
@@ -448,12 +441,7 @@
 	(if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -1048576))
 			   (lt (minus (match_dup 2) (pc)) (const_int 1048572)))
 		  (const_int 4)
-		  (const_int 8)))
-   (set (attr "far_branch")
-	(if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -1048576))
-			   (lt (minus (match_dup 2) (pc)) (const_int 1048572)))
-		  (const_int 0)
-		  (const_int 1)))]
+		  (const_int 8)))]
 )
 
 ;; For a 24-bit immediate CST we can optimize the compare for equality
@@ -670,12 +658,7 @@
 	(if_then_else (and (ge (minus (match_dup 1) (pc)) (const_int -1048576))
 			   (lt (minus (match_dup 1) (pc)) (const_int 1048572)))
 		  (const_int 4)
-		  (const_int 8)))
-   (set (attr "far_branch")
-	(if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -1048576))
-			   (lt (minus (match_dup 2) (pc)) (const_int 1048572)))
-		  (const_int 0)
-		  (const_int 1)))]
+		  (const_int 8)))]
 )
 
 (define_insn "*tb1"
@@ -692,7 +675,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",
 	 "\\t%0, %1, ");
 	else
@@ -709,12 +696,7 @@
 	(if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -32768))
 			   (lt (minus (match_dup 2) (pc)) (const_int 32764)))
 		  (const_int 4)
-		  (const_int 8)))
-   (set (attr "far_branch")
-	(if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -1048576))
-			   (lt (minus (match_dup 2) (pc)) (const_int 1048572)))
-		  (const_int 0)
-		  (const_int 1)))]
+		  (const_int 8)))]
 
 )
 
@@ -727,8 +709,12 @@
   ""
   {
 if (get_attr_length (insn) == 8)
-  {
-	if (get_attr_far_branch (insn) == 1)
+   {
+long long int offset;
+offset = INSN_ADDRESSES (INSN_UID (XEXP (operands[1], 0)))
+		 - INSN_ADDRESSES (INSN_UID (insn));
+
+	if (offset <= -1048576 || offset >= 1048572)
 	  return aarch64_gen_far_branch (operands, 1, "Ltb",
 	 "\\t%0, , ");
 	else
@@ -740,7 +726,7 @@
 	output_asm_insn (buf, operands);
 	return "\t%l1";
 	  }
-  }
+   }
 else
   return "\t%0, , %l1";
   }
@@ -749,12 +735,7 @@
 	(if_then_else (and (ge (minus (match_dup 1) (pc)) (const_int -32768))
 			   (lt (minus (match_dup 1) (pc)) (const_int 32764)))
 		  (const_int 4)
-		  (const_int 8)))
-   (set (attr "far_branch")
-	(if_then_else (and (ge (minus (match_dup 1) (pc)) (const_int -1048576))
-			   (lt (minus (match_dup 1) (pc)) (const_int 1048572)))
-		  (const_int 0)
-		  (const_int 1)))]
+		  (const_int 8)))]
 )
 
 ;; ---