[PATCH] AARCH64_BE load/store rules fix for ARM ABI
akadlec at a-bix.com
Mon Mar 3 03:20:11 PST 2014
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 ?
Suppose the user stores a vector - and then tries to load that to a
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
> 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.
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
>> 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
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.
More information about the llvm-commits