[SVE][CodeGen] Fix TypeSize/ElementCount related warnings in sve-split-store.ll

[david-arm]

Summary

I have fixed up a number of warnings resulting from TypeSize -> uint64_t
casts and calling getVectorNumElements() on scalable vector types. I
think most of the changes are fairly trivial except for those in
DAGTypeLegalizer::SplitVecRes_MSTORE I've tried to ensure we create
the MachineMemoryOperands in a sensible way for scalable vectors.

I have added a CHECK line to the following test:

CodeGen/AArch64/sve-split-store.ll

that ensures no new warnings are added.

[c-rhodes]

I see no CHECK line for warnings in sve-split-store.ll?

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

546

nit: s/StoreSize/StSize for consistency?

[david-arm]

• Renamed StoreSize -> StSize.
• Added check for no warnings in sve-split-store.ll

[david-arm]

ping

[efriedma]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

546

I don't think it's possible for StVT to be a vector type here.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2235

I don't think we should ever get here with a scalable vector? We should guard the creation of DemandedElts.

[david-arm]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

546

I think the final else case covers vectors, right? We can have truncating vector stores. With the way the code is structured we test for the scalar cases first and therefore we were hitting the TypeSize->uint64_t cast warnings before we got to the final else case. That's why I converted the code to using TypeSize here. Alternatively, if you want I can restructure the code along the following lines:

if (StVT.isVector) {

...
return ...;

}

unsigned StWidth = StVT.getSizeInBits().getFixedSize();
unsigned StSize = StVT.getStoreSizeInBits().getFixedSize();
... deal with scalar cases ...

[sdesmalen]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

559

You're using getKnownMinSize() here but using getFixedSize() below on line 560. Is this missing a condition, e.g.:

!StVT.isScalableVector() && !isPowerOf2_64(StWidth.getFixedSize())

or perhaps (because it seems like this only applies to integers):

!StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())

[david-arm]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

559

Yes, I used the known min size here because it could be a scalable vector (since we haven't hit the final else case yet). The equivalent load function has the same problem (see https://reviews.llvm.org/D86697 that was checked in recently). So we either:

1. Restructure the code so that the vector case comes first.
2. Use getKnownMinSize.
3. Use !StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())

I'm happy to go with either way - whichever you and @efriedma prefer really.

[david-arm]

• In 3-op version of GetDemandedBits replaced the isScalableVector check with a check for non-zero DemandedElts.

[sdesmalen]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

559

3 would have my preference.

[efriedma]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

546

We can't have illegal truncating vector stores here; they have to be handled by LegalizeVectorOps.

I guess because of the way the code is structured, we don't check whether the store is legal until later, so we can have a legal truncating vector store here? Which is weird, but I guess you don't need to restructure the code in this patch.

559

3 is fine. Ideally, we'd refactor the code, I guess, but better to keep that separate.

(Theoretically, it's also possible to have a legal truncstore with a three-element vector, although no in-tree target does that. Since we're messing with the logic anyway, might as well handle that correctly.)

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

2457–2458

Do we need to fix Alignment here?

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2235

Is if (!DemandedElts) related to this patch somehow?

[david-arm]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2235

Hi @efriedma, your first comment here said "We should guard the creation of DemandedElts" so I thought this is what you meant. Are you saying that I shouldn't have to change anything at all in this function and that all callers of this function should create an empty DemandedElts for scalable vectors?

[efriedma]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

2235

You shouldn't have to change anything at all; the callers should deal with it.

[david-arm]

• Removed the changes to 3-op version of GetDemandedBits

[david-arm]

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

2457–2458

I'm not sure to be honest. We didn't fix this up in other places (e.g. https://reviews.llvm.org/D86697), such as SpltVecOp_STORE, SplitVecRes_MLOAD. I'm happy to do something with this though if you think it's needed?

[efriedma]

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

2457–2458

I missed it on D86697; we do need the update the alignment.

There's a relationship between the MachinePointerInfo and the specified alignment: the alignment is actually the alignment of the base pointer of the MachinePointerInfo, not the alignment of the operation itself. So anywhere that we're throwing away the MachinePointerInfo, we also need to adjust the alignment to reflect the actual alignment of the operation.

[david-arm]

• Updated alignment for HiMem store when dealing with scalable vectors.

[david-arm]

Hi @efriedma I've tried to update the alignment for the HiMem scalable vector store. Is this new code what you were expecting here? I guess we'll need a new patch to clean up all the other split load/store cases too.

[david-arm]

ping

[sdesmalen]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

553

What guarantees that getFixedSize can be used here?

[david-arm]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

553

I think @eli.friedman mentioned that in an earlier comment that was only for scalar cases, although I agree there are no checks to actually prevent this. I just assumed that we wouldn't be creating arbitrarily large integer types, i.e. 128, 256, etc. I can try adding an extra "!StVT.isVector()" check and see if any tests fail?

[efriedma]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

553

It's implicitly prevented by the way legalization works. In LegalizaeDAG, we assume all vector operations are legal: any illegal vector ops should have been handled by LegalizeVectorOps.

(I don't want to get into a deep discussion of why it works this way, but essentially, the reason it works this way is to allow LegalizeVectorOps to generate operations that involve illegal types.)

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

2449

The ABI alignment isn't relevant here. The relevant numbers are the specified alignment, and the size of LoMemVT. Basically, we need to reduce the alignment if it's bigger than the size of LoMemVT.

I'm sure we have at least one example of this elsewhere.

[david-arm]

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

2449

OK, I'm struggling to know how to proceed here. I've tried again using a function called commonAlignment that works out the minimum of the original alignment and the LoMemVT size. Is this what you mean?

[efriedma]

LGTM

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

2449

This looks right.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

if (!ST->isTruncatingStore()) {
     }
     }
     return;
   }
 
   LLVM_DEBUG(dbgs() << "Legalizing truncating store operations\n");
   SDValue Value = ST->getValue();
   EVT StVT = ST->getMemoryVT();
-  unsigned StWidth = StVT.getSizeInBits();
+  TypeSize StWidth = StVT.getSizeInBits();
+  TypeSize StSize = StVT.getStoreSizeInBits();
   auto &DL = DAG.getDataLayout();
 
-  if (StWidth != StVT.getStoreSizeInBits()) {
+  if (StWidth != StSize) {
     // Promote to a byte-sized store with upper bits zero if not
     // storing an integral number of bytes.  For example, promote
     // TRUNCSTORE:i1 X -> TRUNCSTORE:i8 (and X, 1)
-    EVT NVT = EVT::getIntegerVT(*DAG.getContext(),
-                                StVT.getStoreSizeInBits());
+    EVT NVT = EVT::getIntegerVT(*DAG.getContext(), StSize.getFixedSize());
     Value = DAG.getZeroExtendInReg(Value, dl, StVT);
     SDValue Result =
         DAG.getTruncStore(Chain, dl, Value, Ptr, ST->getPointerInfo(), NVT,
                           ST->getOriginalAlign(), MMOFlags, AAInfo);
     ReplaceNode(SDValue(Node, 0), Result);
-  } else if (StWidth & (StWidth - 1)) {
+  } else if (!StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())) {
     // If not storing a power-of-2 number of bits, expand as two stores.
     assert(!StVT.isVector() && "Unsupported truncstore!");
-    unsigned LogStWidth = Log2_32(StWidth);
+    unsigned StWidthBits = StWidth.getFixedSize();
+    unsigned LogStWidth = Log2_32(StWidthBits);
     assert(LogStWidth < 32);
     unsigned RoundWidth = 1 << LogStWidth;
-    assert(RoundWidth < StWidth);
-    unsigned ExtraWidth = StWidth - RoundWidth;
+    assert(RoundWidth < StWidthBits);
+    unsigned ExtraWidth = StWidthBits - RoundWidth;
     assert(ExtraWidth < RoundWidth);
     assert(!(RoundWidth % 8) && !(ExtraWidth % 8) &&
            "Store size not an integral number of bytes!");
     EVT RoundVT = EVT::getIntegerVT(*DAG.getContext(), RoundWidth);
     EVT ExtraVT = EVT::getIntegerVT(*DAG.getContext(), ExtraWidth);
     SDValue Lo, Hi;
     unsigned IncrementSize;
 

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

SDValue DAGTypeLegalizer::SplitVecOp_MSTORE(MaskedStoreSDNode *N,
 
   EVT MemoryVT = N->getMemoryVT();
   EVT LoMemVT, HiMemVT;
   bool HiIsEmpty = false;
   std::tie(LoMemVT, HiMemVT) =
       DAG.GetDependentSplitDestVTs(MemoryVT, DataLo.getValueType(), &HiIsEmpty);
 
   SDValue Lo, Hi, Res;
+  unsigned LoSize = MemoryLocation::getSizeOrUnknown(LoMemVT.getStoreSize());
   MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
-      N->getPointerInfo(), MachineMemOperand::MOStore, LoMemVT.getStoreSize(),
-      Alignment, N->getAAInfo(), N->getRanges());
+      N->getPointerInfo(), MachineMemOperand::MOStore, LoSize, Alignment,
+      N->getAAInfo(), N->getRanges());
 
   Lo = DAG.getMaskedStore(Ch, DL, DataLo, Ptr, Offset, MaskLo, LoMemVT, MMO,
                           N->getAddressingMode(), N->isTruncatingStore(),
                           N->isCompressingStore());
 
   if (HiIsEmpty) {
     // The hi masked store has zero storage size.
     // Only the lo masked store is needed.
     Res = Lo;
   } else {
 
     Ptr = TLI.IncrementMemoryAddress(Ptr, MaskLo, DL, LoMemVT, DAG,
                                      N->isCompressingStore());
-    unsigned HiOffset = LoMemVT.getStoreSize();
 
+    MachinePointerInfo MPI;
+    if (LoMemVT.isScalableVector()) {
+      Alignment = commonAlignment(
+          Alignment, LoMemVT.getSizeInBits().getKnownMinSize() / 8);
+      MPI = MachinePointerInfo(N->getPointerInfo().getAddrSpace());
+    } else
+      MPI = N->getPointerInfo().getWithOffset(
+          LoMemVT.getStoreSize().getFixedSize());
+
+    unsigned HiSize = MemoryLocation::getSizeOrUnknown(HiMemVT.getStoreSize());
     MMO = DAG.getMachineFunction().getMachineMemOperand(
-        N->getPointerInfo().getWithOffset(HiOffset), MachineMemOperand::MOStore,
-        HiMemVT.getStoreSize(), Alignment, N->getAAInfo(), N->getRanges());
+        MPI, MachineMemOperand::MOStore, HiSize, Alignment, N->getAAInfo(),
+        N->getRanges());
 
     Hi = DAG.getMaskedStore(Ch, DL, DataHi, Ptr, Offset, MaskHi, HiMemVT, MMO,
                             N->getAddressingMode(), N->isTruncatingStore(),
                             N->isCompressingStore());
 
     // Build a factor node to remember that this store is independent of the
     // other one.
     Res = DAG.getNode(ISD::TokenFactor, DL, MVT::Other, Lo, Hi);

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 }
 
 /// See if the specified operand can be simplified with the knowledge that only
 /// the bits specified by DemandedBits are used.
 /// TODO: really we should be making this into the DAG equivalent of
 /// SimplifyMultipleUseDemandedBits and not generate any new nodes.
 SDValue SelectionDAG::GetDemandedBits(SDValue V, const APInt &DemandedBits) {
   EVT VT = V.getValueType();
+
+  if (VT.isScalableVector())
+    return SDValue();
+
   APInt DemandedElts = VT.isVector()
                            ? APInt::getAllOnesValue(VT.getVectorNumElements())
                            : APInt(1, 1);
   return GetDemandedBits(V, DemandedBits, DemandedElts);
 }
 
 /// See if the specified operand can be simplified with the knowledge that only
 /// the bits specified by DemandedBits are used in the elements specified by
 /// DemandedElts.
 /// TODO: really we should be making this into the DAG equivalent of
 /// SimplifyMultipleUseDemandedBits and not generate any new nodes.
 SDValue SelectionDAG::GetDemandedBits(SDValue V, const APInt &DemandedBits,
                                       const APInt &DemandedElts) {
   switch (V.getOpcode()) {
   default:
     return TLI->SimplifyMultipleUseDemandedBits(V, DemandedBits, DemandedElts,
                                                 *this, 0);
   case ISD::Constant: {
     const APInt &CVal = cast<ConstantSDNode>(V)->getAPIntValue();
     APInt NewVal = CVal & DemandedBits;
     if (NewVal != CVal)
       return getConstant(NewVal, SDLoc(V), V.getValueType());
     break;
SDValue SelectionDAG::getTruncStore(SDValue Chain, const SDLoc &dl, SDValue Val,
   MMOFlags |= MachineMemOperand::MOStore;
   assert((MMOFlags & MachineMemOperand::MOLoad) == 0);
 
   if (PtrInfo.V.isNull())
     PtrInfo = InferPointerInfo(PtrInfo, *this, Ptr);
 
   MachineFunction &MF = getMachineFunction();
   MachineMemOperand *MMO = MF.getMachineMemOperand(
-      PtrInfo, MMOFlags, SVT.getStoreSize(), Alignment, AAInfo);
+      PtrInfo, MMOFlags, MemoryLocation::getSizeOrUnknown(SVT.getStoreSize()),
+      Alignment, AAInfo);
   return getTruncStore(Chain, dl, Val, Ptr, SVT, MMO);
 }
 
 SDValue SelectionDAG::getTruncStore(SDValue Chain, const SDLoc &dl, SDValue Val,
                                     SDValue Ptr, EVT SVT,
                                     MachineMemOperand *MMO) {
   EVT VT = Val.getValueType();
 
   assert(Chain.getValueType() == MVT::Other &&
         "Invalid chain type");
   if (VT == SVT)
     return getStore(Chain, dl, Val, Ptr, MMO);
 
   assert(SVT.getScalarType().bitsLT(VT.getScalarType()) &&
          "Should only be a truncating store, not extending!");
   assert(VT.isInteger() == SVT.isInteger() &&
          "Can't do FP-INT conversion!");
   assert(VT.isVector() == SVT.isVector() &&
          "Cannot use trunc store to convert to or from a vector!");
   assert((!VT.isVector() ||
-          VT.getVectorNumElements() == SVT.getVectorNumElements()) &&
+          VT.getVectorElementCount() == SVT.getVectorElementCount()) &&
          "Cannot use trunc store to change the number of vector elements!");
 
   SDVTList VTs = getVTList(MVT::Other);
   SDValue Undef = getUNDEF(Ptr.getValueType());
   SDValue Ops[] = { Chain, Val, Ptr, Undef };
   FoldingSetNodeID ID;
   AddNodeIDNode(ID, ISD::STORE, VTs, Ops);
   ID.AddInteger(SVT.getRawBits());

llvm/test/CodeGen/AArch64/sve-split-store.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
+; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning
 
 ; UNPREDICATED
 
 define void @store_promote_4i8(<vscale x 4 x i8> %data, <vscale x 4 x i8>* %a) {
 ; CHECK-LABEL: store_promote_4i8:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    ptrue p0.s
 ; CHECK-NEXT:    st1b { z0.s }, p0, [x0]

 define void @masked_store_split_8i64(<vscale x 8 x i64> %data, <vscale x 8 x i64> *%a, <vscale x 8 x i1> %pg) {
 ; CHECK-LABEL: masked_store_split_8i64:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    pfalse p1.b
 ; CHECK-NEXT:    zip2 p2.h, p0.h, p1.h
 ; CHECK-NEXT:    zip1 p0.h, p0.h, p1.h
 ; CHECK-NEXT:    zip2 p3.s, p2.s, p1.s
 ; CHECK-NEXT:    zip1 p2.s, p2.s, p1.s
+; CHECK-NEXT:    st1d { z3.d }, p3, [x0, #3, mul vl]
 ; CHECK-NEXT:    st1d { z2.d }, p2, [x0, #2, mul vl]
 ; CHECK-NEXT:    zip2 p2.s, p0.s, p1.s
 ; CHECK-NEXT:    zip1 p0.s, p0.s, p1.s
-; CHECK-NEXT:    st1d { z3.d }, p3, [x0, #3, mul vl]
 ; CHECK-NEXT:    st1d { z1.d }, p2, [x0, #1, mul vl]
 ; CHECK-NEXT:    st1d { z0.d }, p0, [x0]
 ; CHECK-NEXT:    ret
   call void @llvm.masked.store.nxv8i64(<vscale x 8 x i64> %data, <vscale x 8 x i64> *%a, i32 1, <vscale x 8 x i1> %pg)
   ret void
 }
 
 declare void @llvm.masked.store.nxv2i8(<vscale x 2 x i8>, <vscale x 2 x i8>*, i32, <vscale x 2 x i1>)
 declare void @llvm.masked.store.nxv32i8(<vscale x 32 x i8>, <vscale x 32 x i8>*, i32, <vscale x 32 x i1>)
 
 declare void @llvm.masked.store.nxv32i16(<vscale x 32 x i16>, <vscale x 32 x i16>*, i32, <vscale x 32 x i1>)
 
 declare void @llvm.masked.store.nxv8i32(<vscale x 8 x i32>, <vscale x 8 x i32>*, i32, <vscale x 8 x i1>)
 
 declare void @llvm.masked.store.nxv8i64(<vscale x 8 x i64>, <vscale x 8 x i64>*, i32, <vscale x 8 x i1>)