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.