[DAGCombine][RISCV] Don't try to trunc-store combined vector stores

[frasercrmck]

Summary

DAGCombine's mergeStoresOfConstantsOrVecElts optimization is told
whether it's to use vector types and also whether it's to issue a
truncating store. However, the truncating store code path assumes a
scalar integer ConstantSDNode, and when using vector types it creates
either a BUILD_VECTOR or CONCAT_VECTORS to store: neither of which
is a constant.

The riscv64 target is able to expose a crash here because it switches
on both code paths at the same time. The f32 is stored as i32 which
must be promoted to i64, necessitating a truncating store.
It also decides later that it prefers a vector store of v2f32.

While vector truncating stores are legal, this combine is not able to
emit them. We also don't have a test case. This patch adds an assert to
catch this case more gracefully, and updates one of the caller functions
to the function to turn off the use of truncating stores when preferring
vectors.

[craig.topper]

LGTM

[frasercrmck]

• rebase

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 
 bool DAGCombiner::mergeStoresOfConstantsOrVecElts(
     SmallVectorImpl<MemOpLink> &StoreNodes, EVT MemVT, unsigned NumStores,
     bool IsConstantSrc, bool UseVector, bool UseTrunc) {
   // Make sure we have something to merge.
   if (NumStores < 2)
     return false;
 
+  assert((!UseTrunc || !UseVector) &&
+         "This optimization cannot emit a vector truncating store");
+
   // The latest Node in the DAG.
   SDLoc DL(StoreNodes[0].MemNode);
 
   TypeSize ElementSizeBits = MemVT.getStoreSizeInBits();
   unsigned SizeInBits = NumStores * ElementSizeBits;
   unsigned NumMemElts = MemVT.isVector() ? MemVT.getVectorNumElements() : 1;
 
   EVT StoreTy;
for (unsigned i = 0; i < NumConsecutiveStores; ++i) {
                                    *FirstInChain->getMemOperand(), &IsFast) &&
             IsFast)
           LastLegalVectorType = i + 1;
       }
     }
 
     bool UseVector = (LastLegalVectorType > LastLegalType) && AllowVectors;
     unsigned NumElem = (UseVector) ? LastLegalVectorType : LastLegalType;
+    bool UseTrunc = LastIntegerTrunc && !UseVector;
 
     // Check if we found a legal integer type that creates a meaningful
     // merge.
     if (NumElem < 2) {
       // We know that candidate stores are in order and of correct
       // shape. While there is no mergeable sequence from the
       // beginning one may start later in the sequence. The only
       // reason a merge of size N could have failed where another of
while (NumConsecutiveStores >= 2) {
     // Check that we can merge these candidates without causing a cycle.
     if (!checkMergeStoreCandidatesForDependencies(StoreNodes, NumElem,
                                                   RootNode)) {
       StoreNodes.erase(StoreNodes.begin(), StoreNodes.begin() + NumElem);
       NumConsecutiveStores -= NumElem;
       continue;
     }
 
-    MadeChange |= mergeStoresOfConstantsOrVecElts(
-        StoreNodes, MemVT, NumElem, true, UseVector, LastIntegerTrunc);
+    MadeChange |= mergeStoresOfConstantsOrVecElts(StoreNodes, MemVT, NumElem,
+                                                  /*IsConstantSrc*/ true,
+                                                  UseVector, UseTrunc);
 
     // Remove merged stores for next iteration.
     StoreNodes.erase(StoreNodes.begin(), StoreNodes.begin() + NumElem);
     NumConsecutiveStores -= NumElem;
   }
   return MadeChange;
 }
 
if (!checkMergeStoreCandidatesForDependencies(StoreNodes, NumStoresToMerge,
                                                   RootNode)) {
       StoreNodes.erase(StoreNodes.begin(),
                        StoreNodes.begin() + NumStoresToMerge);
       NumConsecutiveStores -= NumStoresToMerge;
       continue;
     }
 
     MadeChange |= mergeStoresOfConstantsOrVecElts(
-        StoreNodes, MemVT, NumStoresToMerge, false, true, false);
+        StoreNodes, MemVT, NumStoresToMerge, /*IsConstantSrc*/ false,
+        /*UseVector*/ true, /*UseTrunc*/ false);
 
     StoreNodes.erase(StoreNodes.begin(), StoreNodes.begin() + NumStoresToMerge);
     NumConsecutiveStores -= NumStoresToMerge;
   }
   return MadeChange;
 }
 
 bool DAGCombiner::tryStoreMergeOfLoads(SmallVectorImpl<MemOpLink> &StoreNodes,

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

bool RISCVTargetLowering::decomposeMulByConstant(LLVMContext &Context, EVT VT,
   }
 
   return false;
 }
 
 bool RISCVTargetLowering::allowsMisalignedMemoryAccesses(
     EVT VT, unsigned AddrSpace, Align Alignment, MachineMemOperand::Flags Flags,
     bool *Fast) const {
-  if (!VT.isScalableVector())
+  if (!VT.isVector())
     return false;
 
   EVT ElemVT = VT.getVectorElementType();
   if (Alignment >= ElemVT.getStoreSize()) {
     if (Fast)
       *Fast = true;
     return true;
   }

llvm/test/CodeGen/RISCV/rvv/combine-store-fp.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+d,+experimental-v -verify-machineinstrs -riscv-v-vector-bits-min=128 < %s | FileCheck %s
+; RUN: llc -mtriple=riscv64 -mattr=+d,+experimental-v -verify-machineinstrs -riscv-v-vector-bits-min=128 < %s | FileCheck %s
+
+define void @combine_fp_zero_stores_crash(float* %ptr)  {
+; CHECK-LABEL: combine_fp_zero_stores_crash:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    addi a0, a0, 4
+; CHECK-NEXT:    vsetivli zero, 2, e32,mf2,ta,mu
+; CHECK-NEXT:    vmv.v.i v25, 0
+; CHECK-NEXT:    vse32.v v25, (a0)
+; CHECK-NEXT:    ret
+  %addr1 = getelementptr float, float * %ptr, i64 1
+  %addr2 = getelementptr float, float * %ptr, i64 2
+  store float 0.000000e+00, float * %addr1, align 4
+  store float 0.000000e+00, float * %addr2, align 4
+  ret void
+}