[CodeGen,AArch64] Fix up warnings in splitStores

[david-arm]

Summary

The code for trying to split up stores is designed for NEON vectors,
where we support arbitrary alignments. It's an optimisation designed
to improve performance by using smaller, aligned stores. However,
we currently only support 16 byte alignments for SVE vectors anyway
so we may as well bail out early.

This change fixes up remaining warnings in a couple of tests:

CodeGen/AArch64/sve-callbyref-notailcall.ll
CodeGen/AArch64/sve-calling-convention-byref.ll

[sdesmalen]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12139

VT.isScalableVector() ?

12141

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?

[david-arm]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12139

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.

12141

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?

[sdesmalen]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12139

Okay that makes sense, thanks for clarifying!

12141

We indeed require an alignment of 16 for scalable vectors allocated on the stack, it's more that given that this code is not doing anything with scalable vectors other than bailing out, having the assert here seems unnecessary. There are already asserts in FrameLowering to cover this.

[sdesmalen]

LGTM!

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue splitStores(SDNode *N, TargetLowering::DAGCombinerInfo &DCI,
                            const AArch64Subtarget *Subtarget) {
 
   StoreSDNode *S = cast<StoreSDNode>(N);
   if (S->isVolatile() || S->isIndexed())
     return SDValue();
 
   SDValue StVal = S->getValue();
   EVT VT = StVal.getValueType();
-  if (!VT.isVector())
+
+  if (!VT.isFixedLengthVector())
     return SDValue();
 
   // If we get a splat of zeros, convert this vector store to a store of
   // scalars. They will be merged into store pairs of xzr thereby removing one
   // instruction and one register.
   if (SDValue ReplacedZeroSplat = replaceZeroVectorStore(DAG, *S))
     return ReplacedZeroSplat;
 
   // FIXME: The logic for deciding if an unaligned store should be split should
   // be included in TLI.allowsMisalignedMemoryAccesses(), and there should be