On Mon 9 Apr, 2018, 2:06 PM Sameera Deshpande, <sameera.deshpa...@linaro.org>
wrote:
> Hi Richard,
>
> I do not see the said patch applied in ToT yet. When do you expect it
> to be available in ToT?
>
> - Thanks and regards,
> Sameera D.
>
> On 30 March 2018 at 17:01, Sameera Deshpande
> <sameera.deshpa...@linaro.org> wrote:
> > Hi Richard,
> >
> > The testcase is working with the patch you suggested, thanks for
> > pointing that out.
> >
> > On 30 March 2018 at 16:54, Sameera Deshpande
> > <sameera.deshpa...@linaro.org> wrote:
> >> On 30 March 2018 at 16:39, Richard Sandiford
> >> <richard.sandif...@linaro.org> 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....@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
> >>>
> >>> [...]
> >>>
> >>>> @@ -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)))
> >>>
> >>> Sorry for not replying earlier, but I think the use of "lt" rather than
> >>> "le" in the current length attribute is deliberate. Distances measured
> >>> from (pc) in "length" are a bit special in that backward distances are
> >>> measured from the start of the instruction and forward distances are
> >>> measured from the end of the instruction:
> >>>
> >>> /* The address of the current insn. We implement this actually
> as the
> >>> address of the current insn for backward branches, but the
> last
> >>> address of the next insn for forward branches, and both with
> >>> adjustments that account for the worst-case possible
> stretching of
> >>> intervening alignments between this insn and its
> destination. */
> >>>
> >>> This avoids the chicken-and-egg situation of the length of the branch
> >>> depending on the forward distance and the forward distance depending
> >>> on the length of the branch.
> >>>
> >>> In contrast, this code:
> >>>
> >>>> @@ -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
> >>>
> >>> is reading the final computed addresses, so the code is right to use
> >>> the real instruction range. (FWIW I agree with Kyrill that using
> >>> IN_RANGE with hex constants would be clearer.)
> >>>
> >>> That said... a possible problem comes from situations like:
> >>>
> >>> address length insn
> >>> ......c 8 A
> >>> ..align to 8 bytes...
> >>> ......8 B
> >>> ......c 4 C
> >>> ..align to 16 bytes...
> >>> ......0 D, branch to B
> >>>
> >>> when D is at the maximum extent of the branch range and when GCC's
> length
> >>> for A is only a conservative estimate. If the length of A turns out to
> >>> be 4 rather than 8 at assembly time, the alignment to 8 bytes will be
> >>> a no-op and the address of B and C will be 8 less than we calculated.
> >>> But the alignment to 16 bytes will then insert 8 bytes of padding
> rather
> >>> than none, and the address of D won't change. The branch will then be
> >>> out of range even though the addresses calculated by GCC showed it as
> >>> being in range. insn_current_reference_address accounts for this, and
> >>> so copes correctly with conservative lengths.
> >>>
> >>> The length can legitimately be conservative for things like asm
> >>> statements, so I guess using the far_branch attribute is best
> >>> after all. Sorry for the wrong turn.
> >>>
> >>> On the face of it (without access to the testcase), the only problem
> >>> with using far_branch in the output template is that insn_last_address
> >>> is not set, but needs to be for insn_current_reference_address to do
> >>> the right thing for forward branches. Does the patch below work for
> >>> your testcase?
> >>>
> >>> (As the documentation you mentioned in the original covering message
> >>> says, it wouldn't be correct to use far_branch in anything other
> >>> than final, since only the "length" attribute can respond to changes
> >>> in addresses while the lengths are being calculated. But using it
> >>> in final should be fine.)
> >>>
> >>> Thanks,
> >>> Richard
> >>>
> >>>
> >>> 2018-03-31 Richard Sandiford <richard.sandif...@linaro.org>
> >>>
> >>> gcc/
> >>> * final.c (final_1): Set insn_last_address to the same value as
> >>> insn_current_address.
> >>>
> >>> Index: gcc/final.c
> >>> ===================================================================
> >>> --- gcc/final.c 2018-03-30 11:54:02.058881968 +0100
> >>> +++ gcc/final.c 2018-03-30 11:54:02.228869184 +0100
> >>> @@ -2081,6 +2081,9 @@ final_1 (rtx_insn *first, FILE *file, in
> >>> }
> >>> else
> >>> insn_current_address = INSN_ADDRESSES (INSN_UID (insn));
> >>> + /* final can be seen as an iteration of shorten_branches that
> >>> + does nothing (since a fixed point has already been
> reached). */
> >>> + insn_last_address = insn_current_address;
> >>> }
> >>>
> >>> dump_basic_block_info (file, insn, start_to_bb, end_to_bb,
> >>
> >> Hi Richard,
> >>
> >> Thanks for the explanation. The problem was indeed because correct
> >> insn_last_address was not set properly, because of which the attribute
> >> FAR_BRANCH didn't work appropriately. However, I am not sure if that
> >> was the only problem. Will check the testcase with the ToT sans my
> >> changes, and will revert.
> >>
> >> --
> >> - Thanks and regards,
> >> Sameera D.
> >
> >
> >
> > --
> > - Thanks and regards,
> > Sameera D.
>
>
>
Hi Richard,
Can this patch be backported to GCC7?
--
> - Thanks and regards,
> Sameera D.
>
