[AArch64] Fix incorrect big-endian spill in foldMemoryOperandImpl

[john-brawn-arm]

When an sreg sub-register of a q register was spilled, AArch64InstrInfo::foldMemoryOperandImpl would emit a spill of a d register, which gives the wrong result when the target is big-endian as the following q register fill will put the value in the top half.

Fix this by greatly simplifying the existing code for widening the spill to only handle wzr to xzr widening, as the default result we get if the function returns nullptr is already that a widened spill will be emitted.

[john-brawn-arm]

Ping.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 25da11541c0740950e812a85d332a6d5cf4a5f87 d40f30d780bca7071235939aee7392b9474f2f4a -- llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index caca784fec03..818a691181c0 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -10,8 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "AArch64ExpandImm.h"
 #include "AArch64InstrInfo.h"
+#include "AArch64ExpandImm.h"
 #include "AArch64FrameLowering.h"
 #include "AArch64MachineFunctionInfo.h"
 #include "AArch64Subtarget.h"

[john-brawn-arm]

Ping.

[davemgreen]

Sorry I missed this - github isn't a great system at times.

This will create larger stores than we had in the past? Is it worth making this "little endian only", or are the smaller stores not worth it, when you start considering the load-store forwarding and that kind of thing?

And does the IsFill code below have similar problems, or is it already valid?

[john-brawn-arm]

Sorry I missed this - github isn't a great system at times.

This will create larger stores than we had in the past? Is it worth making this "little endian only", or are the smaller stores not worth it, when you start considering the load-store forwarding and that kind of thing?

We only get larger stores in cases where previously a spill of a 32-bit sub-register of a 128-bit register was widened to a 64-bit spill (and now becomes a 128-bit spill). If we want to do a smaller store it would be better to do it deliberately so e.g. the above case would become a 32-bit spill.

And does the IsFill code below have similar problems, or is it already valid?

That's fine because there the spill slot is the same size as the sub-register destination of the COPY and it's not changing the size of the fill, just doing it to the destination of the COPY instead of the source, saving a copy. We're missing a test though for the opposite case (a spill slot larger than the sub-register destination of the COPY), so I'll add that.

[davemgreen]

I think this sounds OK, given store-load forwarding and whatnot.

Are there some tests specifically for BE?

[john-brawn-arm]

Are there some tests specifically for BE?

I've added a big-endian run to the spill-fold.mir test.

[davemgreen]

Thanks. And all the check lines remain the same? In that case this LGTM