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 <[email protected]> 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 <[email protected]> wrote:
>>>
>>> On 15/03/18 15:27, Sameera Deshpande wrote:
>>>>
>>>>
>>>> Ping!
>>>>
>>>> On 28 February 2018 at 16:18, Sameera Deshpande
>>>> <[email protected]> wrote:
>>>>>
>>>>>
>>>>> On 27 February 2018 at 18:25, Ramana Radhakrishnan
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>> On Wed, Feb 14, 2018 at 8:30 AM, Sameera Deshpande
>>>>>> <[email protected]> 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 <[email protected]>
>>>>>>> * 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)))]
)
;; -------------------------------------------------------------------
