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

[Albrecht Kadlec]

On 2014-02-27 12:27, Tim Northover wrote:
> Hi Albrecht,
> 
>>   1) disable LDx/STx
> 
> I still think this is misguided; the lane and duplicating instructions
> are very different beasts, without any suggestion (as far as I can
> see) that they're problematic except that they share part of their
> mnemonic with the dodgy ones.

Does the user expect different behaviour in LE & BE ?
probably not.

Suppose the user stores a vector - and then tries to load that to a 
lane.

Results in different layout in LE & BE.

The correct fix would have been to use different C data types for 
reversed and array vector memory layout*) and the respective 
nonterminals in the matcher - with the corresponding conversion chain 
rules in a matcher that applies them in the cheapest locations , or the 
code will suck big time from the conversion swaps.

That also needs add/sub/.... rules for both types of nonterminals - for 
all the operations that work lane-wise, so that the matcher has more 
leeway to move the swaps to cold locations.

*) that means different intrinsics return types in LE & BE.
Good luck with expecting correct use, if >90% of the people work on LE, 
where casting-mistakes won't show.

As always the correct solution is difficult & expensive once the problem 
was generated.


> Anything disabled should be because you can explain why it goes wrong,
> not through guilt by association.

That was the first step towards working code.
Normally only working rules are added to the system.
Here I had many potentially detrimental rules for BE.

In the meantime, I had the time to think about all that.
Watch out for the upcoming diff.


>>   4) optionally enable LDR/STR for LE
> 
> I think this should happen first. I realise your main concern is
> big-endian support, but it's best not to get too bogged down in that
> if there's an opportunity to improve other parts of LLVM at the same
> time. Since it looks like these are more flexible generally, and
> identical to ld1 for LE, I'd suggest a separate patch adding these
> patterns for all both BE & LE (with tests, obviously).
> 
> I'd support that change from what I've heard. *If* people object, then
> we can go back to this suggestion (reluctantly). I think that's
> unlikely though.

Done.
Only had to fix all respective LE unit tests :-(
Good thing is: they're also BE tests now.


>>   Ideally these would be separate patches each - but matcher fails 
>> don't go down well.
> 
> Agreed, but I'm not too bothered by that personally. I'm not sure
> where you're getting matcher fails with a partial patch though; that
> might need looking into if it's in the regression tests.

Removing LD1 without adding LDR leaves vector register loads 
unmatchable.

>> Then I added the new patterns conservatively for BE only - don't want 
>> to change LE
>> code just now (we're comparing LE to BE, for example). Also not going 
>> to ruffle feathers
>> of any LE guys (one pandora's box at the same time)
> 
> I'd say you're more likely to do that by restricting all changes to BE
> than by changing LE when it's a benefit to both.
> 
> Finally, the newest patch still doesn't have any regression tests. The
> other stuff's definitely still up for debate, but those really are
> essential.

BE neon ld/st now covered by same tests as LE.
+ new LE/BE tests for so far un-covered vector types - shocking: 
somebody's not been doing his unit-tests! :-)

New patch upcoming today.

Cheers,
Albrecht