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

[Tim Northover]

Forwarding this to the list since it wasn't CCed at the time:

  Hi Albrecht,

  Thanks for working on this.

  I think you've been a bit too liberal with your IsLE predicate,
applying it to both patterns that you don't want to disable on BE (if
I've understood properly) and to instruction definitions without any
patterns (currently harmless, but pointless too).

  I've also made one comment about the IsBE use. Is it really necessary?

  Finally, there should definitely be regression tests for changes
like this. Preferably for each pattern you're introducing or changing.
(This is particularly important at the moment because (as Chris said)
eventually we'd like to merge Apple's ARM64 LLVM port with this one,
which will mean big changes everywhere).

  Cheers.

  Tim.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4901
@@ +4900,3 @@
+  // float is always there
+  // shouldn't this be guarded by HasFPARMv8 ???
+  defm : ls_neutral_pats<LOAD, STORE, Base, Offset, address, f64>;
----------------
If the CPU doesn't have FPARMv8 then f64 won't be a legal type and the
DAG shouldn't contain any instances of it by this time.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4904
@@ +4903,3 @@
+
+  // TODO: eventually also enable for LE
+  let Predicates = [HasNEON, IsBE] in {
----------------
LLVM normally uses "FIXME" rather than "TODO". A consistent choice
makes grepping a bit easier.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4858-4859
@@ +4857,4 @@
+
+// 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
+// TODO: eventually also enable for LE
----------------
It'd probably be a good idea to refer people to the AAPCS here for more details.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4856
@@ +4855,3 @@
+
+// wrappers to instantiate all allowed same-size fp/vector loads
+
----------------
Could we have capital letters at the start of sentences?

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3391
@@ -3383,1 +3390,3 @@
+// this should work in BE - single operand: no element swaps
+let Predicates = [IsLE] in {
 def LD1_1D : NeonI_LDVList<0, 0b0111, 0b11, VOne1D_operand, "ld1">;
----------------
There aren't any patterns in this multiclass so this is superfluous.
If patterns *are* added, it's not clear that they'll be wrong for BE
either: they could be the int_arm_neon_vldN version which you do want.

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3888
@@ -3866,2 +3887,3 @@

+let Predicates = [IsLE] in {
 // Load single 1-element structure to all lanes of 1 register
----------------
No patterns and you probably *would* want any that existed since the
layout issue doesn't exist for the duplicating loads.

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3907
@@ +3906,3 @@
+
+let Predicates = [IsLE] in {
+// element swap on bytes == byte swap
----------------
Ah, here they are. I think these patterns should be endian-independent.

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3996
@@ -3966,3 +3995,3 @@

-// Load single 1-element structure to one lane of 1 register.
-defm LD1LN : LDN_Lane_BHSD<0b0, 0b0, "VOne", "ld1">;
+let Predicates = [IsLE] in {
+  // Load single 1-element structure to one lane of 1 register.
----------------
Loading a single lane is also layout independent (and these are not
the patterns).

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4893
@@ +4892,3 @@
+    defm : ls_neutral_pats<LOAD, STORE, Base, Offset, address, v1i32>;
+//    defm : ls_neutral_pats<LOAD, STORE, Base, Offset, address,
v1f32>; does not exist - v1f64 DOES --  WHY ?
+  }
----------------
Commented code.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4860
@@ +4859,3 @@
+// LD1 & ST1 are not ABI conforming in big endian: wrong arg memory layout
+// TODO: eventually also enable for LE
+// (desired by ARM - smaller code due to more powerful adressing modes)
----------------
Why is this only eventually? Couldn't it be enabled now if it's got
better addressing-mode properties?


http://llvm-reviews.chandlerc.com/D2884