[Aarch64] Fix conditional branches with target far away.
Kyrill Tkachov
kyrylo.tkachov@foss.arm.com
Thu Mar 29 15:54:00 GMT 2018
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
More information about the Gcc-patches
mailing list