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....@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....@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.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >
> >
> >
>
>
