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

Tim Northover t.p.northover at gmail.com
Mon Mar 3 23:33:24 PST 2014


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

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

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

Cheers.

Tim.



More information about the llvm-commits mailing list