Hi Sudakshina, Thanks for pointing that out. Updated the conditions for attribute length to take care of boundary conditions for offset range.
Please find attached the updated patch. I have tested it for gcc testsuite and the failing testcase. Ok for trunk? On 22 March 2018 at 19:06, Sudakshina Das <sudi....@arm.com> wrote: > Hi Sameera > > On 22/03/18 02:07, Sameera Deshpande wrote: >> >> Hi Sudakshina, >> >> As per the ARMv8 ARM, for the offset range (-1048576 ,1048572), the >> far branch instruction offset is inclusive of both the offsets. Hence, >> I am using <=||=> and not <||>= as it was in previous implementation. > > > I have to admit earlier I was only looking at the patch mechanically and > found a difference with the previous implementation in offset comparison. > After you pointed out, I looked up the ARMv8 ARM and I have a couple of > doubts: > > 1. My understanding is that any offset in [-1048576 ,1048572] both inclusive > qualifies as an 'in range' offset. However, the code for both attribute > length and far_branch has been using [-1048576 ,1048572), that is, ( >= && < > ). If the far_branch was incorrectly calculated, then maybe the length > calculations with similar magic numbers should also be corrected? Of course, > I am not an expert in this and maybe this was a conscience decision so I > would ask Ramana to maybe clarify if he remembers. > > 2. Now to come back to your patch, if my understanding is correct, I think a > far_branch would be anything outside of this range, that is, > (offset < -1048576 || offset > 1048572), anything that can not be > represented in the 21-bit range. > > Thanks > Sudi > > >> >> On 16 March 2018 at 00:51, Sudakshina Das <sudi....@arm.com> wrote: >>> >>> On 15/03/18 15:27, Sameera Deshpande wrote: >>>> >>>> >>>> Ping! >>>> >>>> On 28 February 2018 at 16:18, Sameera Deshpande >>>> <sameera.deshpa...@linaro.org> wrote: >>>>> >>>>> >>>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan >>>>> <ramana....@googlemail.com> wrote: >>>>>> >>>>>> >>>>>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande >>>>>> <sameera.deshpa...@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.deshpa...@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)))] ) ;; -------------------------------------------------------------------