* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-29 11:28 ` Sameera Deshpande
@ 2018-03-29 15:33 ` Sudakshina Das
2018-03-29 15:35 ` Sameera Deshpande
2018-03-29 15:54 ` Kyrill Tkachov
2018-03-30 11:24 ` Richard Sandiford
2 siblings, 1 reply; 17+ messages in thread
From: Sudakshina Das @ 2018-03-29 15:33 UTC (permalink / raw)
To: Sameera Deshpande
Cc: Ramana Radhakrishnan, James Greenhalgh, Marcus Shawcroft,
Richard Earnshaw, Richard Sandiford, gcc-patches, nd
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 <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
>>
>>
>>>
>>> On 16 March 2018 at 00:51, Sudakshina Das <sudi.das@arm.com> wrote:
>>>>
>>>> On 15/03/18 15:27, Sameera Deshpande wrote:
>>>>>
>>>>>
>>>>> Ping!
>>>>>
>>>>> On 28 February 2018 at 16:18, Sameera Deshpande
>>>>> <sameera.deshpande@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>>>>>> <ramana.gcc@googlemail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>>>>>>> <sameera.deshpande@linaro.org> 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 <sameera.deshpande@linaro.org>
>>>>>>>> * 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",
>>>> "<inv_tb>\\t%<w>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.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-29 15:33 ` Sudakshina Das
@ 2018-03-29 15:35 ` Sameera Deshpande
0 siblings, 0 replies; 17+ messages in thread
From: Sameera Deshpande @ 2018-03-29 15:35 UTC (permalink / raw)
To: Sudakshina Das
Cc: Ramana Radhakrishnan, James Greenhalgh, Marcus Shawcroft,
Richard Earnshaw, Richard Sandiford, gcc-patches, nd
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, <sudi.das@arm.com> 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 <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
> >>
> >>
> >>>
> >>> On 16 March 2018 at 00:51, Sudakshina Das <sudi.das@arm.com> wrote:
> >>>>
> >>>> On 15/03/18 15:27, Sameera Deshpande wrote:
> >>>>>
> >>>>>
> >>>>> Ping!
> >>>>>
> >>>>> On 28 February 2018 at 16:18, Sameera Deshpande
> >>>>> <sameera.deshpande@linaro.org> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan
> >>>>>> <ramana.gcc@googlemail.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
> >>>>>>> <sameera.deshpande@linaro.org> 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 <sameera.deshpande@linaro.org>
> >>>>>>>> * 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",
> >>>> "<inv_tb>\\t%<w>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.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-29 11:28 ` Sameera Deshpande
2018-03-29 15:33 ` Sudakshina Das
@ 2018-03-29 15:54 ` Kyrill Tkachov
2018-03-30 11:24 ` Richard Sandiford
2 siblings, 0 replies; 17+ messages in thread
From: Kyrill Tkachov @ 2018-03-29 15:54 UTC (permalink / raw)
To: Sameera Deshpande, Sudi Das
Cc: Ramana Radhakrishnan, James Greenhalgh, Marcus Shawcroft,
Richard Earnshaw, Richard Sandiford, gcc-patches, nd
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 <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
> >
> >
> >>
> >> On 16 March 2018 at 00:51, Sudakshina Das <sudi.das@arm.com> wrote:
> >>>
> >>> On 15/03/18 15:27, Sameera Deshpande wrote:
> >>>>
> >>>>
> >>>> Ping!
> >>>>
> >>>> On 28 February 2018 at 16:18, Sameera Deshpande
> >>>> <sameera.deshpande@linaro.org> wrote:
> >>>>>
> >>>>>
> >>>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan
> >>>>> <ramana.gcc@googlemail.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
> >>>>>> <sameera.deshpande@linaro.org> 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 <sameera.deshpande@linaro.org>
> >>>>>>> * 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?
> >>>
<snip>
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 "predicated" "yes,no" (const_string "no"))
@@ -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)))
- (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
@@ -688,14 +676,9 @@
[(set_attr "type" "branch")
(set (attr "length")
(if_then_else (and (ge (minus (match_dup 1) (pc)) (const_int -1048576))
- (lt (minus (match_dup 1) (pc)) (const_int 1048572)))
+ (le (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 "*tb<optab><mode>1"
@@ -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, ");
INSN_ADDRESSES returns an "int", so the long long type is not necessary, but I guess there's no harm from it.
Since we're moving these constant from the RTL attributes into C code we should take the opportunity
to write these numbers in a more meaningful form and make use of GCC helper functions for improved readability.
I'd write this check as:
/* If the offset doesn't fit in the signed 21-bit range. */
if (!IN_RANGE (offset,-0x100000, 0xffffc)) // please double-check my hex conversion!
else
@@ -727,14 +714,9 @@
[(set_attr "type" "branch")
(set (attr "length")
(if_then_else (and (ge (minus (match_dup 2) (pc)) (const_int -32768))
- (lt (minus (match_dup 2) (pc)) (const_int 32764)))
+ (le (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)))]
)
@@ -747,8 +729,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",
"<inv_tb>\\t%<w>0, <sizem1>, ");
else
Likewise on this hunk.
Thanks,
Kyrill
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-29 11:28 ` Sameera Deshpande
2018-03-29 15:33 ` Sudakshina Das
2018-03-29 15:54 ` Kyrill Tkachov
@ 2018-03-30 11:24 ` Richard Sandiford
2018-03-30 11:31 ` Sameera Deshpande
2 siblings, 1 reply; 17+ messages in thread
From: Richard Sandiford @ 2018-03-30 11:24 UTC (permalink / raw)
To: Sameera Deshpande
Cc: Sudakshina Das, Ramana Radhakrishnan, James Greenhalgh,
Marcus Shawcroft, Richard Earnshaw, gcc-patches, nd
> 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,
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-30 11:24 ` Richard Sandiford
@ 2018-03-30 11:31 ` Sameera Deshpande
2018-03-30 12:18 ` Sameera Deshpande
0 siblings, 1 reply; 17+ messages in thread
From: Sameera Deshpande @ 2018-03-30 11:31 UTC (permalink / raw)
To: Sameera Deshpande, Sudakshina Das, Ramana Radhakrishnan,
James Greenhalgh, Marcus Shawcroft, Richard Earnshaw,
gcc-patches, nd, Richard Sandiford
On 30 March 2018 at 16:39, Richard Sandiford
<richard.sandiford@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.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,
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-30 11:31 ` Sameera Deshpande
@ 2018-03-30 12:18 ` Sameera Deshpande
2018-04-09 8:36 ` Sameera Deshpande
0 siblings, 1 reply; 17+ messages in thread
From: Sameera Deshpande @ 2018-03-30 12:18 UTC (permalink / raw)
To: Sameera Deshpande, Sudakshina Das, Ramana Radhakrishnan,
James Greenhalgh, Marcus Shawcroft, Richard Earnshaw,
gcc-patches, nd, Richard Sandiford
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.deshpande@linaro.org> wrote:
> On 30 March 2018 at 16:39, Richard Sandiford
> <richard.sandiford@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.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,
>
> 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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Aarch64] Fix conditional branches with target far away.
2018-03-30 12:18 ` Sameera Deshpande
@ 2018-04-09 8:36 ` Sameera Deshpande
2018-07-31 11:50 ` Sameera Deshpande
0 siblings, 1 reply; 17+ messages in thread
From: Sameera Deshpande @ 2018-04-09 8:36 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
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.deshpande@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.deshpande@linaro.org> wrote:
>> On 30 March 2018 at 16:39, Richard Sandiford
>> <richard.sandiford@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.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,
>>
>> 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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Aarch64] Fix conditional branches with target far away.
2018-04-09 8:36 ` Sameera Deshpande
@ 2018-07-31 11:50 ` Sameera Deshpande
0 siblings, 0 replies; 17+ messages in thread
From: Sameera Deshpande @ 2018-07-31 11:50 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
On Mon 9 Apr, 2018, 2:06 PM Sameera Deshpande, <sameera.deshpande@linaro.org>
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
> <sameera.deshpande@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.deshpande@linaro.org> wrote:
> >> On 30 March 2018 at 16:39, Richard Sandiford
> >> <richard.sandiford@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.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,
> >>
> >> 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.
>
>
>
Hi Richard,
Can this patch be backported to GCC7?
--
> - Thanks and regards,
> Sameera D.
>
^ permalink raw reply [flat|nested] 17+ messages in thread