[PATCH] D80720: [CodeGen,AArch64] Fix up warnings in splitStores

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 3 inline comments as done.
david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12091
+
+  if (!VT.isFixedLengthVector()) {
+    // All SVE vectors should be aligned to 16 bytes
----------------
sdesmalen wrote:
> `VT.isScalableVector()` ?
The original code bailed out if it wasn't a vector, now we want to bail if it's not a fixed length vector. That's why I can't just use

if (VT.isScalableVector())

as it would then allow non-vector types in.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12093
+    // All SVE vectors should be aligned to 16 bytes
+    assert(!VT.isScalableVector() || S->getAlignment() == 16 &&
+           "Alignment is not 16 bytes for scalable vectors!");
----------------
sdesmalen wrote:
> It's a bit odd to assert `!VT.isScalableVector()` here given the condition above, but I also don't really see why you're asserting the alignment here in this function. What was the motivation for adding it?
So we can get here for two cases:

1) It's not a vector at all, hence the assert(!VT.isScalableVector() || ..)
2) It's a scalable vector. This function has been specifically written to optimise cases of NEON vector stores to addresses with terrible alignments, i.e. not 16 bytes. It's hard to split up stores for SVE, although not impossible - and the cost of doing so probably makes it not worth it. However, we shouldn't even have to worry about mis-alignments for SVE vectors at all because they are always supposed to be aligned to 16 bytes. That's why I put the assert in, although I admit it may be unnecessary. I can remove it if you want?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80720/new/

https://reviews.llvm.org/D80720