[PATCH] AARCH64_BE load/store rules fix for ARM ABI
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
+// 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
+// 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
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.
More information about the llvm-commits