| Message ID | CAAdirjy_9vvKfM38d-U64f+MPNLeR3x3-JkKWg_Erf0+LCJSAg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Aarch64] Fix conditional branches with target far away. | expand |
On 14 February 2018 at 14:00, 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. > > -- > - Thanks and regards, > Sameera D. Gentle reminder! -- - Thanks and regards, Sameera D.
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.
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. -- - Thanks and regards, Sameera D.
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. > > -- > - Thanks and regards, > Sameera D. -- - Thanks and regards, Sameera D.
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. > > >
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 <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. >> >> >> >> > -- - Thanks and regards, Sameera D.
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. >>> >>> >>> >>> >> > > >
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'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. >>>> >>>> >>>> >>>> >>>> >>> >> >> >> > -- - Thanks and regards, Sameera D. 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, "); 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 @@ -760,7 +746,7 @@ output_asm_insn (buf, operands); return "<bcond>\t%l1"; } - } + } else return "<tbz>\t%<w>0, <sizem1>, %l1"; } @@ -767,14 +753,9 @@ [(set_attr "type" "branch") (set (attr "length") (if_then_else (and (ge (minus (match_dup 1) (pc)) (const_int -32768)) - (lt (minus (match_dup 1) (pc)) (const_int 32764))) + (le (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)))] ) ;; -------------------------------------------------------------------
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. >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>> >>> >>> >> > > >
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. > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > >>> > >> > > > > > > > >
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
> 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,
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.
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, 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.
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. >
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 "*tb<optab><mode>1" @@ -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", "<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)))] ) @@ -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", "<inv_tb>\\t%<w>0, <sizem1>, "); else @@ -740,7 +726,7 @@ output_asm_insn (buf, operands); return "<bcond>\t%l1"; } - } + } else return "<tbz>\t%<w>0, <sizem1>, %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)))] ) ;; -------------------------------------------------------------------
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. -- - Thanks and regards, Sameera D.