[PATCH] AARCH64_BE load/store rules fix for ARM ABI

[James Molloy]

>
> true for the last part -> changed that to
> // Also enabled for LE
> // (desired by ARM - smaller code due to more powerful adressing modes)
> to deter other reviewers ;-)
> For the first part: I still want that here - at the start of the LDR/STR
> rules, because it is the reason these rules have been added for BE and LE.
> It's non-obvious, why NEON register loads are handled along with normal
> loads.
> A lot of that paragraph is there by request from James Molloy (other
> reviewer).
> BTW: I just noticed he was dropped from the CCs.


I'm still here :)

I still don't like the "desired by ARM" statement. Enabling both LDR and
LD1 for loads is obviously correct, and obviously better than just one (as
you have a wider choice of addressing modes). I don't think what ARM wants
should come into it, and I wouldn't like this statement to make it into the
codebase.

Thanks for doing the other changes I requested!

Cheers,

James


On 4 March 2014 11:38, Tim Northover <t.p.northover at gmail.com> wrote:

> Hi Albrecht,
>
> On 4 March 2014 10:19, Albrecht Kadlec <akadlec at a-bix.com> wrote:
> > For the first part: I still want that here - at the start of the LDR/STR
> > rules, because it is the reason these rules have been added for BE and
> LE.
> > It's non-obvious, why NEON register loads are handled along with normal
> > loads.
>
> Well, fair enough.
>
> > Could we move back to Phabricator for the comments on the "final few"
> diffs
> > ?
>
> Yep, sounds good to me.
>
> > Sorry to add to your pain: Then we'd logically have to remove the IsBE
> > predicate: "Don't add what you can't test". We have no other way of
> ensuring
> > that the code paths stay the same for LE & BE - and that adding a rule
> for
> > LE doesn't kill BE.
>
> I'm actually much more worried about people introducing unwarranted
> similarities between BE and LE than the reverse, which carpet-bombing
> the tests with an identical aarch64_be run line is not going to guard
> against. In fact it would be more likely cement any unjustified
> symmetry.
>
> Anyone explicitly introducing some dependence on IsBE or IsLE will be
> expected to provide tests for that functionality in both directions.
>
> > Unfortunately there's no concept of lower priority weekly tests, that I
> know
> > of. (asan would like that too - Hi Kostya!)
>
> You could try separate files with "REQUIRES: long_tests". I think one
> or two people might actually run them, but not many. Another
> reasonably common approach would be to *change* some of the existing
> aarch64 tests to aarch64_be in areas that are almost certainly common
> (this is how linux & iOS tend to share the 32-bit ARM tests, for
> example).
>
> > So I'm fine with removing the identical tests for BE, if you tell me
> that BE
> > is not such an important target that it has to be guarded against
> > unintentional changes.
>
> I think it's not different enough to warrant doubling the test load
> for. Similar arguments could be made for PIC vs static, large/small
> code models, NEON/no-NEON or any number of other options that affect
> how the backend behaves (we're up to "* 16" already).
>
> But the reality is that they have focussed tests on the areas they're
> expected to affect. Testing whether we can add two integers
> successfully in PIC mode or make use of "fmla" when the code is
> destined for the JIT just isn't productive.
>
> The blanket approach has proven itself ineffective already, before
> even being committed, with:
>
> > neon-copy.ll is one of those that we need to investigate. Why would you
> say
> > that it's wrong - it's just hard-coded to LE ?
>
> Yes, take the first one for example:
>
> define <16 x i8> @ins16bw(<16 x i8> %tmp1, i8 %tmp2) {
> ;CHECK: ins {{v[0-9]+}}.b[15], {{w[0-9]+}}
>   %tmp3 = insertelement <16 x i8> %tmp1, i8 %tmp2, i32 15
>   ret <16 x i8> %tmp3
> }
>
> On big-endian I believe that should produce "ins v0.b[0], w0" because
> that "i32 15" refers to the element of the vector with the highest
> address (if it was stored). The rest are the same, as well as most
> (but probably not all) other uses of indexes in the tests.
>
> > 1) remove IsLE guards for lane-loads - also for any parts of LD1 ???
>
> The "normal" ld1s are probably fine as you have them. I wouldn't worry
> about being selective there for now, since mostly ldr does the same
> job.
>
> > 2) remove AARCH64_BE test RUNs that are equivalent to LE, because BE
> isn't
> > important enough to be tested all the time.
>
> I'd be reasonably happy for you to change a few from LE to BE (and
> just add BE in some important edge cases) *after* careful thought. But
> it's a separate issue to this patch, so perhaps it would be best to
> discuss it in a separate thread.
>
> This patch specifically tweaks some load/store functionality, so
> that's what it should be testing.
>
> > 3) anything else from another thread I forgot ?
>
> Not that I can think of.
>
> Cheers.
>
> Tim.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140304/bbfed9be/attachment.html>