check for fastness before merging in DAGCombiner::MergeConsecutiveStores()

[spatel]

Summary

This is another step on the long road to fixing the last part of PR21711:
https://llvm.org/bugs/show_bug.cgi?id=21711

This change was mentioned in:
http://reviews.llvm.org/D10662 and
http://reviews.llvm.org/D10905

The change in DAGCombiner is simple: use and check the 'IsFast' optional parameter to TLI.allowsMemoryAccess() any time we have a merged access candidate.

But I think that change exposes a bug in the AMD/SI implementation of allowsMisalignedMemoryAccesses(). A test failure shows up in test/CodeGen/AMDGPU/merge-stores.ll. Matt, can you confirm/deny that that part of the patch is what you expect?

[spatel]

Forgot to add llvm-commits.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

412–415 ↗

(On Diff #33855)

This looks fine to me

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

for (unsigned i=0; i<LastConsecutiveStore+1; ++i) {
       } else {
         // Non-constant.
         break;
       }
 
       // Find a legal type for the constant store.
       unsigned SizeInBits = (i+1) * ElementSizeBytes * 8;
       EVT StoreTy = EVT::getIntegerVT(Context, SizeInBits);
+      bool IsFast;
       if (TLI.isTypeLegal(StoreTy) &&
           TLI.allowsMemoryAccess(Context, DL, StoreTy, FirstStoreAS,
-                                 FirstStoreAlign)) {
+                                 FirstStoreAlign, &IsFast) && IsFast) {
         LastLegalType = i+1;
       // Or check whether a truncstore is legal.
       } else if (TLI.getTypeAction(Context, StoreTy) ==
                  TargetLowering::TypePromoteInteger) {
         EVT LegalizedStoredValueTy =
           TLI.getTypeToTransformTo(Context, StoredVal.getValueType());
         if (TLI.isTruncStoreLegal(LegalizedStoredValueTy, StoreTy) &&
             TLI.allowsMemoryAccess(Context, DL, LegalizedStoredValueTy,
-                                   FirstStoreAS, FirstStoreAlign)) {
+                                   FirstStoreAS, FirstStoreAlign, &IsFast) &&
+            IsFast) {
           LastLegalType = i + 1;
         }
       }
 
       // We only use vectors if the constant is known to be zero or the target
       // allows it and the function is not marked with the noimplicitfloat
       // attribute.
       if ((!NonZero || TLI.storeOfVectorConstantIsCheap(MemVT, i+1,
                                                         FirstStoreAS)) &&
           !NoVectors) {
         // Find a legal type for the vector store.
         EVT Ty = EVT::getVectorVT(Context, MemVT, i+1);
         if (TLI.isTypeLegal(Ty) &&
             TLI.allowsMemoryAccess(Context, DL, Ty, FirstStoreAS,
-                                   FirstStoreAlign))
+                                   FirstStoreAlign, &IsFast) && IsFast)
           LastLegalVectorType = i + 1;
       }
     }
 
     // Check if we found a legal integer type to store.
     if (LastLegalType == 0 && LastLegalVectorType == 0)
       return false;
 
for (unsigned i = 0; i < LastConsecutiveStore + 1; ++i) {
       // 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 (StoredVal.getOpcode() != ISD::EXTRACT_VECTOR_ELT)
         return false;
 
       // Find a legal type for the vector store.
       EVT Ty = EVT::getVectorVT(Context, MemVT, i+1);
+      bool IsFast;
       if (TLI.isTypeLegal(Ty) &&
           TLI.allowsMemoryAccess(Context, DL, Ty, FirstStoreAS,
-                                 FirstStoreAlign))
+                                 FirstStoreAlign, &IsFast) && IsFast)
         NumElem = i + 1;
     }
 
     return MergeStoresOfConstantsOrVecElts(StoreNodes, MemVT, NumElem,
                                            false, true);
   }
 
   // Below we handle the case of multiple consecutive stores that
for (unsigned i = 1; i < LoadNodes.size(); ++i) {
     // All loads much share the same chain.
     if (LoadNodes[i].MemNode->getChain() != FirstChain)
       break;
 
     int64_t CurrAddress = LoadNodes[i].OffsetFromBase;
     if (CurrAddress - StartAddress != (ElementSizeBytes * i))
       break;
     LastConsecutiveLoad = i;
-
     // Find a legal type for the vector store.
     EVT StoreTy = EVT::getVectorVT(Context, MemVT, i+1);
+    bool IsFastSt, IsFastLd;
     if (TLI.isTypeLegal(StoreTy) &&
         TLI.allowsMemoryAccess(Context, DL, StoreTy, FirstStoreAS,
-                               FirstStoreAlign) &&
+                               FirstStoreAlign, &IsFastSt) && IsFastSt &&
         TLI.allowsMemoryAccess(Context, DL, StoreTy, FirstLoadAS,
-                               FirstLoadAlign)) {
+                               FirstLoadAlign, &IsFastLd) && IsFastLd) {
       LastLegalVectorType = i + 1;
     }
 
     // Find a legal type for the integer store.
     unsigned SizeInBits = (i+1) * ElementSizeBytes * 8;
     StoreTy = EVT::getIntegerVT(Context, SizeInBits);
     if (TLI.isTypeLegal(StoreTy) &&
         TLI.allowsMemoryAccess(Context, DL, StoreTy, FirstStoreAS,
-                               FirstStoreAlign) &&
+                               FirstStoreAlign, &IsFastSt) && IsFastSt &&
         TLI.allowsMemoryAccess(Context, DL, StoreTy, FirstLoadAS,
-                               FirstLoadAlign))
+                               FirstLoadAlign, &IsFastLd) && IsFastLd)
       LastLegalIntegerType = i + 1;
     // Or check whether a truncstore and extload is legal.
     else if (TLI.getTypeAction(Context, StoreTy) ==
              TargetLowering::TypePromoteInteger) {
       EVT LegalizedStoredValueTy =
         TLI.getTypeToTransformTo(Context, StoreTy);
       if (TLI.isTruncStoreLegal(LegalizedStoredValueTy, StoreTy) &&
           TLI.isLoadExtLegal(ISD::ZEXTLOAD, LegalizedStoredValueTy, StoreTy) &&
           TLI.isLoadExtLegal(ISD::SEXTLOAD, LegalizedStoredValueTy, StoreTy) &&
           TLI.isLoadExtLegal(ISD::EXTLOAD, LegalizedStoredValueTy, StoreTy) &&
           TLI.allowsMemoryAccess(Context, DL, LegalizedStoredValueTy,
-                                 FirstStoreAS, FirstStoreAlign) &&
+                                 FirstStoreAS, FirstStoreAlign, &IsFastSt) &&
+          IsFastSt &&
           TLI.allowsMemoryAccess(Context, DL, LegalizedStoredValueTy,
-                                 FirstLoadAS, FirstLoadAlign))
+                                 FirstLoadAS, FirstLoadAlign, &IsFastLd) &&
+          IsFastLd)
         LastLegalIntegerType = i+1;
     }
   }
 
   // Only use vector types if the vector type is larger than the integer type.
   // If they are the same, use integers.
   bool UseVectorTy = LastLegalVectorType > LastLegalIntegerType && !NoVectors;
   unsigned LastLegalType = std::max(LastLegalVectorType, LastLegalIntegerType);

llvm/trunk/lib/Target/AMDGPU/SIISelLowering.cpp

bool SITargetLowering::allowsMisalignedMemoryAccesses(EVT VT,
 
   // XXX - The only mention I see of this in the ISA manual is for LDS direct
   // reads the "byte address and must be dword aligned". Is it also true for the
   // normal loads and stores?
   if (AddrSpace == AMDGPUAS::LOCAL_ADDRESS) {
     // ds_read/write_b64 require 8-byte alignment, but we can do a 4 byte
     // aligned, 8 byte access in a single operation using ds_read2/write2_b32
     // with adjacent offsets.
-    return Align % 4 == 0;
+    bool AlignedBy4 = (Align % 4 == 0);
+    if (IsFast)
+      *IsFast = AlignedBy4;
+    return AlignedBy4;
   }
 
   // Smaller than dword value must be aligned.
   // FIXME: This should be allowed on CI+
   if (VT.bitsLT(MVT::i32))
     return false;
 
   // 8.1.6 - For Dword or larger reads or writes, the two LSBs of the

llvm/trunk/test/CodeGen/X86/dag-merge-fast-accesses.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=-slow-unaligned-mem-16 | FileCheck %s --check-prefix=FAST
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+slow-unaligned-mem-16 | FileCheck %s --check-prefix=SLOW
+
+; Verify that the DAGCombiner is creating unaligned 16-byte loads and stores
+; if and only if those are fast.
+
+define void @merge_const_vec_store(i64* %ptr) {
+; FAST-LABEL: merge_const_vec_store:
+; FAST:       # BB#0:
+; FAST-NEXT:    xorps %xmm0, %xmm0
+; FAST-NEXT:    movups %xmm0, (%rdi)
+; FAST-NEXT:    retq
+;
+; SLOW-LABEL: merge_const_vec_store:
+; SLOW:       # BB#0:
+; SLOW-NEXT:    movq $0, (%rdi)
+; SLOW-NEXT:    movq $0, 8(%rdi)
+; SLOW-NEXT:    retq
+
+  %idx0 = getelementptr i64, i64* %ptr, i64 0
+  %idx1 = getelementptr i64, i64* %ptr, i64 1
+
+  store i64 0, i64* %idx0, align 8
+  store i64 0, i64* %idx1, align 8
+  ret void
+}
+
+
+define void @merge_vec_element_store(<4 x double> %v, double* %ptr) {
+; FAST-LABEL: merge_vec_element_store:
+; FAST:       # BB#0:
+; FAST-NEXT:    movups %xmm0, (%rdi)
+; FAST-NEXT:    retq
+;
+; SLOW-LABEL: merge_vec_element_store:
+; SLOW:       # BB#0:
+; SLOW-NEXT:    movlpd %xmm0, (%rdi)
+; SLOW-NEXT:    movhpd %xmm0, 8(%rdi)
+; SLOW-NEXT:    retq
+
+  %vecext0 = extractelement <4 x double> %v, i32 0
+  %vecext1 = extractelement <4 x double> %v, i32 1
+
+  %idx0 = getelementptr double, double* %ptr, i64 0
+  %idx1 = getelementptr double, double* %ptr, i64 1
+
+  store double %vecext0, double* %idx0, align 8
+  store double %vecext1, double* %idx1, align 8
+  ret void
+}
+
+
+define void @merge_vec_load_and_stores(i64 *%ptr) {
+; FAST-LABEL: merge_vec_load_and_stores:
+; FAST:       # BB#0:
+; FAST-NEXT:    movups (%rdi), %xmm0
+; FAST-NEXT:    movups %xmm0, 40(%rdi)
+; FAST-NEXT:    retq
+;
+; SLOW-LABEL: merge_vec_load_and_stores:
+; SLOW:       # BB#0:
+; SLOW-NEXT:    movq (%rdi), %rax
+; SLOW-NEXT:    movq 8(%rdi), %rcx
+; SLOW-NEXT:    movq %rax, 40(%rdi)
+; SLOW-NEXT:    movq %rcx, 48(%rdi)
+; SLOW-NEXT:    retq
+
+  %idx0 = getelementptr i64, i64* %ptr, i64 0
+  %idx1 = getelementptr i64, i64* %ptr, i64 1
+
+  %ld0 = load i64, i64* %idx0, align 4
+  %ld1 = load i64, i64* %idx1, align 4
+
+  %idx4 = getelementptr i64, i64* %ptr, i64 5
+  %idx5 = getelementptr i64, i64* %ptr, i64 6
+
+  store i64 %ld0, i64* %idx4, align 4
+  store i64 %ld1, i64* %idx5, align 4
+  ret void
+}
+