[DAG] Relax type restriction for store merge

[niravd]

Summary

Allow stores of bitcastable types to be merged by peeking through BITCAST nodes and recasting stored values constant and vector extract nodes as necessary.

[niravd]

Add missing check for typesize equality and explicitly avoid truncating stores.

[niravd]

Resolve outstanding comments and rebase over latned extracted NFCI patch.

[niravd]

Ignore this last patch. It should have gone to another diff. Will overwrite with correct version presently.

[RKSimon]

This looks like the D34472 patch?

[niravd]

overwrite accidental patch.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12509

Val.getOpcode()

12949

StVal.getOpcode()

[niravd]

Simpify, add comments, and address nits discussed.

[niravd]

Addressed missed nits.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12643

It'd probably be better if you avoid the ternary inside the if().

if (MemVT.isInteger() && !MemVT.bitsEq(Other->getMemoryVT()))
   return false;
if (!MemVT.isInteger() && Other->getMemoryVT() != MemVT)
   return false;

or

if ((MemVT.isInteger() && !MemVT.bitsEq(Other->getMemoryVT())) ||
    (!MemVT.isInteger() && Other->getMemoryVT() != MemVT))
   return false;

That or pull it out as a bool variable in the line above.

12655

Another ternary inside an if()

[niravd]

extract ternary operators from conditionals

[niravd]

Cleanup the mergestore helper function casting and add missing extract_subvector casting logic.

[niravd]

Rework to regularize and simplify bitcasting logic.

[niravd]

Rebase past landed NFC commits.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

483–484

A lot of these comment changes look like NFCs that can go in separately.

12482

Please can you create a static helper that performs this? We have a lot of peek through bitcasts in this patch alone....

12484

Val.getValueSizeInBits()

12518

Merge these ifs() to reduce indentation?

if (MemVT != Val.getValueType() &&
    (Val.getOpcode() == ISD::EXTRACT_VECTOR_ELT ||
     Val.getOpcode() == ISD::EXTRACT_SUBVECTOR) {

[niravd]

Pull out some NFC changes and use peekThroughBitcast function

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12487

Why isn't this CFP->getValueAPF().bitcastToAPInt().zextOrTrunc(8 * ElementSizeBytes) ?

12642

Pull the ternary operator out of the if()

12665

NFC change

[niravd]

Excise NFC change and address comments.

[RKSimon]

LGTM - thanks

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

private:
     /// Helper function for MergeConsecutiveStores which merges the
     /// component store chains.
     SDValue getMergeStoreChains(SmallVectorImpl<MemOpLink> &StoreNodes,
                                 unsigned NumStores);
 
     /// This is a helper function for MergeConsecutiveStores. When the
     /// source elements of the consecutive stores are all constants or
     /// all extracted vector elements, try to merge them into one
-    /// larger store.  \return True if a merged store was created.
+    /// larger store introducing bitcasts if necessary.  \return True
+    /// if a merged store was created.
     bool MergeStoresOfConstantsOrVecElts(SmallVectorImpl<MemOpLink> &StoreNodes,
                                          EVT MemVT, unsigned NumStores,
                                          bool IsConstantSrc, bool UseVector,
                                          bool UseTrunc);
 
     /// This is a helper function for MergeConsecutiveStores. Stores
     /// that potentially may be merged with St are placed in
     /// StoreNodes.
     void getStoreMergeCandidates(StoreSDNode *St,
                                  SmallVectorImpl<MemOpLink> &StoreNodes);
 
     /// Helper function for MergeConsecutiveStores. Checks if
     /// candidate stores have indirect dependency through their
     /// operands. \return True if safe to merge.
     bool checkMergeStoreCandidatesForDependencies(
         SmallVectorImpl<MemOpLink> &StoreNodes, unsigned NumStores);
 
     /// Merge consecutive store operations into a wide store.
     /// This optimization uses wide integers or vectors when possible.
     /// \return number of stores that were merged into a merged store (the
     /// affected nodes are stored as a prefix in \p StoreNodes).
     bool MergeConsecutiveStores(StoreSDNode *N);
bool DAGCombiner::MergeStoresOfConstantsOrVecElts(
 
   SDValue StoredVal;
   if (UseVector) {
     if (IsConstantSrc) {
       SmallVector<SDValue, 8> BuildVector;
       for (unsigned I = 0; I != NumStores; ++I) {
         StoreSDNode *St = cast<StoreSDNode>(StoreNodes[I].MemNode);
         SDValue Val = St->getValue();
-        if (MemVT.getScalarType().isInteger())
+        // If constant is of the wrong type, convert it now.
+        if (MemVT != Val.getValueType()) {
+          Val = peekThroughBitcast(Val);
+          // Deal with constants of wrong size.
+          if (ElementSizeBytes * 8 != Val.getValueSizeInBits()) {
+            EVT IntMemVT =
+                EVT::getIntegerVT(*DAG.getContext(), MemVT.getSizeInBits());
             if (auto *CFP = dyn_cast<ConstantFPSDNode>(Val))
               Val = DAG.getConstant(
-                (uint32_t)CFP->getValueAPF().bitcastToAPInt().getZExtValue(),
-                SDLoc(CFP), MemVT);
+                  CFP->getValueAPF().bitcastToAPInt().zextOrTrunc(
+                      8 * ElementSizeBytes),
+                  SDLoc(CFP), IntMemVT);
+            else if (auto *C = dyn_cast<ConstantSDNode>(Val))
+              Val = DAG.getConstant(
+                  C->getAPIntValue().zextOrTrunc(8 * ElementSizeBytes),
+                  SDLoc(C), IntMemVT);
+          }
+          // Make sure correctly size type is the correct type.
+          Val = DAG.getBitcast(MemVT, Val);
+        }
         BuildVector.push_back(Val);
       }
-      StoredVal = DAG.getBuildVector(StoreTy, DL, BuildVector);
+      StoredVal = DAG.getNode(MemVT.isVector() ? ISD::CONCAT_VECTORS
+                                               : ISD::BUILD_VECTOR,
+                              DL, StoreTy, BuildVector);
     } else {
       SmallVector<SDValue, 8> Ops;
       for (unsigned i = 0; i < NumStores; ++i) {
         StoreSDNode *St = cast<StoreSDNode>(StoreNodes[i].MemNode);
-        SDValue Val = St->getValue();
-        // All operands of BUILD_VECTOR / CONCAT_VECTOR must have the same type.
-        if (Val.getValueType() != MemVT)
-          return false;
+        SDValue Val = peekThroughBitcast(St->getValue());
+        // All operands of BUILD_VECTOR / CONCAT_VECTOR must be of
+        // type MemVT. If the underlying value is not the correct
+        // type, but it is an extraction of an appropriate vector we
+        // can recast Val to be of the correct type. This may require
+        // converting between EXTRACT_VECTOR_ELT and
+        // EXTRACT_SUBVECTOR.
+        if ((MemVT != Val.getValueType()) &&
+            (Val.getOpcode() == ISD::EXTRACT_VECTOR_ELT ||
+             Val.getOpcode() == ISD::EXTRACT_SUBVECTOR)) {
+          SDValue Vec = Val.getOperand(0);
+          EVT MemVTScalarTy = MemVT.getScalarType();
+          // We may need to add a bitcast here to get types to line up.
+          if (MemVTScalarTy != Vec.getValueType()) {
+            unsigned Elts = Vec.getValueType().getSizeInBits() /
+                            MemVTScalarTy.getSizeInBits();
+            EVT NewVecTy =
+                EVT::getVectorVT(*DAG.getContext(), MemVTScalarTy, Elts);
+            Vec = DAG.getBitcast(NewVecTy, Vec);
+          }
+          auto OpC = (MemVT.isVector()) ? ISD::EXTRACT_SUBVECTOR
+                                        : ISD::EXTRACT_VECTOR_ELT;
+          Val = DAG.getNode(OpC, SDLoc(Val), MemVT, Vec, Val.getOperand(1));
+        }
         Ops.push_back(Val);
       }
 
       // Build the extracted vector elements back into a vector.
       StoredVal = DAG.getNode(MemVT.isVector() ? ISD::CONCAT_VECTORS
                                                : ISD::BUILD_VECTOR,
                               DL, StoreTy, Ops);
     }
if (UseVector) {
     StoredVal = DAG.getConstant(StoreInt, DL, StoreTy);
   }
 
   LSBaseSDNode *FirstInChain = StoreNodes[0].MemNode;
   SDValue NewChain = getMergeStoreChains(StoreNodes, NumStores);
 
   // make sure we use trunc store if it's necessary to be legal.
   SDValue NewStore;
-  if (UseVector || !UseTrunc) {
+  if (!UseTrunc) {
     NewStore = DAG.getStore(NewChain, DL, StoredVal, FirstInChain->getBasePtr(),
                             FirstInChain->getPointerInfo(),
                             FirstInChain->getAlignment());
   } else { // Must be realized as a trunc store
     EVT LegalizedStoredValueTy =
         TLI.getTypeToTransformTo(*DAG.getContext(), StoredVal.getValueType());
     unsigned LegalizedStoreSize = LegalizedStoredValueTy.getSizeInBits();
     ConstantSDNode *C = cast<ConstantSDNode>(StoredVal);

 
 void DAGCombiner::getStoreMergeCandidates(
     StoreSDNode *St, SmallVectorImpl<MemOpLink> &StoreNodes) {
   // This holds the base pointer, index, and the offset in bytes from the base
   // pointer.
   BaseIndexOffset BasePtr = BaseIndexOffset::match(St->getBasePtr(), DAG);
   EVT MemVT = St->getMemoryVT();
 
-  SDValue Val = St->getValue();
+  SDValue Val = peekThroughBitcast(St->getValue());
   // We must have a base and an offset.
   if (!BasePtr.getBase().getNode())
     return;
 
   // Do not handle stores to undef base pointers.
   if (BasePtr.getBase().isUndef())
     return;
 
if (IsLoadSrc) {
     // Load and store should be the same type.
     if (MemVT != LoadVT)
       return;
   }
   auto CandidateMatch = [&](StoreSDNode *Other, BaseIndexOffset &Ptr,
                             int64_t &Offset) -> bool {
     if (Other->isVolatile() || Other->isIndexed())
       return false;
-    SDValue Val = Other->getValue();
+    SDValue Val = peekThroughBitcast(Other->getValue());
+    // Allow merging constants of different types as integers.
+    bool NoTypeMatch = (MemVT.isInteger()) ? !MemVT.bitsEq(Other->getMemoryVT())
+                                           : Other->getMemoryVT() != MemVT;
     if (IsLoadSrc) {
-      // Loads must match type.
-      if (Other->getMemoryVT() != MemVT)
+      if (NoTypeMatch)
         return false;
       // The Load's Base Ptr must also match
       if (LoadSDNode *OtherLd = dyn_cast<LoadSDNode>(Val)) {
         auto LPtr = BaseIndexOffset::match(OtherLd->getBasePtr(), DAG);
         if (LoadVT != OtherLd->getMemoryVT())
           return false;
         if (!(LBasePtr.equalBaseIndex(LPtr, DAG)))
           return false;
       } else
         return false;
     }
     if (IsConstantSrc) {
-      // Allow merging constants of different types as integers.
-      if (MemVT.isInteger() ? !MemVT.bitsEq(Other->getMemoryVT())
-                            : Other->getMemoryVT() != MemVT)
+      if (NoTypeMatch)
         return false;
       if (!(isa<ConstantSDNode>(Val) || isa<ConstantFPSDNode>(Val)))
         return false;
     }
     if (IsExtractVecSrc) {
-      // Must match type.
-      if (Other->getMemoryVT() != MemVT)
+      // Do not merge truncated stores here.
+      if (Other->isTruncatingStore())
+        return false;
+      if (!MemVT.bitsEq(Val.getValueType()))
         return false;
       if (Val.getOpcode() != ISD::EXTRACT_VECTOR_ELT &&
           Val.getOpcode() != ISD::EXTRACT_SUBVECTOR)
         return false;
     }
     Ptr = BaseIndexOffset::match(Other->getBasePtr(), DAG);
     return (BasePtr.equalBaseIndex(Ptr, DAG, Offset));
   };
   // We looking for a root node which is an ancestor to all mergable
   // stores. We search up through a load, to our root and then down
   // through all children. For instance we will find Store{1,2,3} if
bool DAGCombiner::MergeConsecutiveStores(StoreSDNode *St) {
   if (ElementSizeBytes * 8 != MemVT.getSizeInBits())
     return false;
 
   if (!MemVT.isSimple())
     return false;
 
   // Perform an early exit check. Do not bother looking at stored values that
   // are not constants, loads, or extracted vector elements.
-  SDValue StoredVal = St->getValue();
+  SDValue StoredVal = peekThroughBitcast(St->getValue());
   bool IsLoadSrc = isa<LoadSDNode>(StoredVal);
   bool IsConstantSrc = isa<ConstantSDNode>(StoredVal) ||
                        isa<ConstantFPSDNode>(StoredVal);
   bool IsExtractVecSrc = (StoredVal.getOpcode() == ISD::EXTRACT_VECTOR_ELT ||
                           StoredVal.getOpcode() == ISD::EXTRACT_SUBVECTOR);
 
   if (!IsConstantSrc && !IsLoadSrc && !IsExtractVecSrc)
     return false;
while (StoreNodes.size() > 1) {
     // in one vector store rather than a sequence of scalar stores.
     if (IsExtractVecSrc) {
       LSBaseSDNode *FirstInChain = StoreNodes[0].MemNode;
       unsigned FirstStoreAS = FirstInChain->getAddressSpace();
       unsigned FirstStoreAlign = FirstInChain->getAlignment();
       unsigned NumStoresToMerge = 1;
       for (unsigned i = 0; i < NumConsecutiveStores; ++i) {
         StoreSDNode *St = cast<StoreSDNode>(StoreNodes[i].MemNode);
-        SDValue StVal = St->getValue();
+        SDValue StVal = peekThroughBitcast(St->getValue());
         // This restriction could be loosened.
         // Bail out if any stored values are not elements extracted from a
         // vector. It should be possible to handle mixed sources, but load
         // sources need more careful handling (see the block of code below that
         // handles consecutive loads).
         if (StVal.getOpcode() != ISD::EXTRACT_VECTOR_ELT &&
             StVal.getOpcode() != ISD::EXTRACT_SUBVECTOR)
           return RV;
 
         // Find a legal type for the vector store.
while (StoreNodes.size() > 1) {
     // Look for load nodes which are used by the stored values.
     SmallVector<MemOpLink, 8> LoadNodes;
 
     // Find acceptable loads. Loads need to have the same chain (token factor),
     // must not be zext, volatile, indexed, and they must be consecutive.
     BaseIndexOffset LdBasePtr;
     for (unsigned i = 0; i < NumConsecutiveStores; ++i) {
       StoreSDNode *St = cast<StoreSDNode>(StoreNodes[i].MemNode);
-      SDValue Val = St->getValue();
+      SDValue Val = peekThroughBitcast(St->getValue());
       LoadSDNode *Ld = dyn_cast<LoadSDNode>(Val);
       if (!Ld)
         break;
 
       // Loads must only have one use.
       if (!Ld->hasNUsesOfValue(1, 0))
         break;
 

test/CodeGen/X86/MergeConsecutiveStores.ll

 
 ; CHECK-LABEL: merge_vec_extract_stores
 ; CHECK:      vmovups %ymm0, 48(%rdi)
 ; CHECK-NEXT: vmovups %ymm1, 80(%rdi)
 ; CHECK-NEXT: vzeroupper
 ; CHECK-NEXT: retq
 }
 
-; Merging vector stores when sourced from vector loads is not currently handled.
+; Merging vector stores when sourced from vector loads.
 define void @merge_vec_stores_from_loads(<4 x float>* %v, <4 x float>* %ptr) {
   %load_idx0 = getelementptr inbounds <4 x float>, <4 x float>* %v, i64 0
   %load_idx1 = getelementptr inbounds <4 x float>, <4 x float>* %v, i64 1
   %v0 = load <4 x float>, <4 x float>* %load_idx0
   %v1 = load <4 x float>, <4 x float>* %load_idx1
   %store_idx0 = getelementptr inbounds <4 x float>, <4 x float>* %ptr, i64 0
   %store_idx1 = getelementptr inbounds <4 x float>, <4 x float>* %ptr, i64 1
   store <4 x float> %v0, <4 x float>* %store_idx0, align 16
define void @merge_bitcast(<4 x i32> %v, float* %ptr) {
   %idx3 = getelementptr inbounds float, float* %ptr, i64 3
   store float %f0, float* %idx0, align 4
   store float %f1, float* %idx1, align 4
   store float %f2, float* %idx2, align 4
   store float %f3, float* %idx3, align 4
   ret void
 
 ; CHECK-LABEL: merge_bitcast
-; CHECK:      vmovd %xmm0, (%rdi)
-; CHECK-NEXT: vpextrd $1, %xmm0, 4(%rdi)
-; CHECK-NEXT: vpextrd $2, %xmm0, 8(%rdi)
-; CHECK-NEXT: vpextrd $3, %xmm0, 12(%rdi)
+; CHECK:      vmovups %xmm0, (%rdi)
 ; CHECK-NEXT: retq
 }