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

Tim Northover t.p.northover at gmail.com
Thu Feb 27 03:27:00 PST 2014


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.

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

>   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.

>   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.

> 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.

Cheers.

Tim.



More information about the llvm-commits mailing list