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

[Albrecht Kadlec]

On 2014-03-04 08:33, Tim Northover wrote:
>> Ok, if you LGTM that, I'll change the predicates to comments and let 
>> someone
>> else find the failing patterns.
>> We can always hide behind "undefined behaviour" signs when the mob 
>> arrives.
> 
> OK, just a few comments on the newest patch. I think some of the
> comments are out of date given recent changes. This block, in
> particular:
> 
> +// NEON-BE: allow all neon vectors as well, since ld1/st1 must be 
> disabled
> +// LD1 & ST1 are not ABI conforming in big endian: wrong arg memory 
> layout
> +//
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
> +// section 4.1.2, 2nd paragraph: LDR/STR layout
> +// "on a big-endian system element 0 will contain the 
> highest-addressed
> +// element of a short vector."
> +// FIXME: eventually also enable for LE
> +// (desired by ARM - smaller code due to more powerful adressing 
> modes)
> 
> The last bit is no longer true and the first bit probably belongs at
> the start of the ldN/stN block since they're the ones that are treated
> specially.

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.
Also we seem to have several threads running by now, which is bound to 
lead to hiccups - this one, for example, somehow doesn't show up in 
Phabricator any more - despite the CC :-(
Could we move back to Phabricator for the comments on the "final few" 
diffs ?


There is another (bigger) block at the beginning of the LDn/STn rules to 
explain that other part of the change - & in more detail.


> It pains me to say it, but I think you might have gone a bit far now
> in adding your tests, for 2 reasons:
> 
> 1. The code paths are almost entirely the same, and we don't really
> have the resources to do a full cartesian product of all
> configurations (these tests are run multiple times daily by everyone).

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.
These already are the minimum set of unit smoke tests - not even near 
full coverage.

For me BE is a different backend, that will eventually break, if it 
isn't tested regularly (doesn't need to be nightly).

Unfortunately there's no concept of lower priority weekly tests, that I 
know of. (asan would like that too - Hi Kostya!)

Resources aside:
IsLE has the potential to introduce totally different behaviour for BE 
(I do that for some parts).

Why would we test all of LE against unintentional side effects from 
matcher changes - and none of BE?
I guess I could just sit back and wait for someone to add a rule for LE, 
that breaks BE.

Resources:
make check-all still passes in 21 seconds for LE & BE:
(I've been using test suites that ran all night for end-to-end 
compiling/linking/simulating a few thousand files)

$ time make check-all
Testing Time: 19.07s
   Expected Passes    : 5643
   Expected Failures  : 17
   Unsupported Tests  : 4518

real	0m19.561s
user	0m55.477s
sys	0m44.766s

That said: some ToDos have been exposed for the caling conventions patch 
by this exercise (128 bit params that have the two w-regs swapped).
That'll be submitted together with the upcoming calling convention patch 
as it's unrelated to LDn/STn)

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.

We'll keep the failed ones though, once they're fixed, so the exercise 
wasn't totally futile.


> 2. As I mentioned, lanes are screwy in the implementation of
> big-endian you're pursuing so I think many of those tests are actually
> wrong on big-endian ("neon-copy.ll" for example, but it's not the only
> one).

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 ?


> Wrong tests are worse than non-existent ones.
> 
> I'd prefer a more targeted approach, covering the areas where we
> expect real differences. In this case, we're only really changing
> vector loads & stores. Your added file is probably sufficient.

ok, that and the few files that have different calling conventions in BE 
(i128 args).


> Could you upload the new version before committing? Just to give it
> one final check over.

sure, but I'll wait for your replies first, so I know what exactly to 
do:

AFAIK:
1) remove IsLE guards for lane-loads - also for any parts of LD1 ???
2) remove AARCH64_BE test RUNs that are equivalent to LE, because BE 
isn't important enough to be tested all the time.
3) anything else from another thread I forgot ?

Cheers,
Albrecht