[MemCpyOpt] Fix wrong merging of adjacent nontemporal stores into memset calls.

[andreadb]

Summary

Pass MemCpyOpt doesn't check if a store instructions is nontemporal.
As a consequence, adjacent nontemporal stores may be wrongly merged into memset calls.

Example:

define void @foo(<4 x float>* nocapture %dst) {
entry:
  store <4 x float> zeroinitializer, <4 x float>* %dst, align 16, !nontemporal !0
  %ptr1 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 1
  store <4 x float> zeroinitializer, <4 x float>* %ptr1, align 16, !nontemporal !0
  ret void
}

!0 = !{i32 1}

In this example, the two nontemporal stores are combined to a memset of zero which does not preserve the nontemporal hint. Later on the backend (tested on my x86-64 corei7) expands that memset call into a sequence of two normal 16-byte aligned vector stores.

opt -memcpyopt foo.ll -S -o - | llc -mcpu=corei7 -o -

Before:

xorps  %xmm0, %xmm0
movaps  %xmm0, 16(%rdi)
movaps  %xmm0, (%rdi)

With this patch, we no longer merge nontemporal stores into calls to memset.
In this example, llc correctly expands the two stores into two movntps:

xorps  %xmm0, %xmm0
movntps %xmm0, 16(%rdi)
movntps  %xmm0, (%rdi)

Please let me know if okay to submit.

Thanks,
Andrea

[qcolombet]

lib/Transforms/Scalar/MemCpyOptimizer.cpp

495 ↗

(On Diff #36761)

Would it make sense tp extend the metadata to allow this propagation to happen?
I believe the problem here is that we need to differentiate between the load or the store being non-temporal.

[andreadb]

lib/Transforms/Scalar/MemCpyOptimizer.cpp

495 ↗

(On Diff #36761)

Hi Quentin,

Are you suggesting to extend the usage of !nontemporal metadata to memcpy/memset calls as well? If my understanding is correct then yes, that would definitely enable the merging of nontemporal stores.

However - correct me if I am wrong - I am under the impression that a change like that would only have the effect of forcing the backend to expand !nontemporal memsets back to sequences of store instructions.
A memset library call would not have the same semantic of a builtin !nontemporal memset call. So, SelectionDAG will have to conservatively expand it back to a sequence of !nontemporal stores (effectively undoing the merging).
So, unless I got it completely wrong (which may be the case :-) ), I find it difficult to justify a change like that one.

What do you think?

[qcolombet]

Hi Andrea,

I am under the impression that a change like that would only have the effect of forcing the backend to expand !nontemporal memsets back to sequences of store instructions.

That’s a good point.
Add that in the comment so that we remember why we made this design decision :).

LGTM.

Thanks,
Q.

llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp

Instruction *MemCpyOpt::tryMergingIntoMemset(Instruction *StartInst,
   }
 
   return AMemSet;
 }
 
 
 bool MemCpyOpt::processStore(StoreInst *SI, BasicBlock::iterator &BBI) {
   if (!SI->isSimple()) return false;
+
+  // Avoid merging nontemporal stores since the resulting
+  // memcpy/memset would not be able to preserve the nontemporal hint.
+  // In theory we could teach how to propagate the !nontemporal metadata to
+  // memset calls. However, that change would force the backend to
+  // conservatively expand !nontemporal memset calls back to sequences of
+  // store instructions (effectively undoing the merging).
+  if (SI->getMetadata(LLVMContext::MD_nontemporal))
+    return false;
+
   const DataLayout &DL = SI->getModule()->getDataLayout();
 
   // Detect cases where we're performing call slot forwarding, but
   // happen to be using a load-store pair to implement it, rather than
   // a memcpy.
   if (LoadInst *LI = dyn_cast<LoadInst>(SI->getOperand(0))) {
     if (LI->isSimple() && LI->hasOneUse() &&
         LI->getParent() == SI->getParent()) {

llvm/trunk/test/Transforms/MemCpyOpt/nontemporal.ll

+; RUN: opt < %s -memcpyopt -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+; Verify that we don't combine nontemporal stores into memset calls.
+
+define void @nontemporal_stores_1(<4 x float>* nocapture %dst) {
+; CHECK-LABEL: @nontemporal_stores_1
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %dst, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr1, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr2, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr3, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr4, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr5, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr6, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr7, align 16, !nontemporal !0
+; CHECK-NEXT: ret void
+entry:
+  store <4 x float> zeroinitializer, <4 x float>* %dst, align 16, !nontemporal !0
+  %ptr1 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 1
+  store <4 x float> zeroinitializer, <4 x float>* %ptr1, align 16, !nontemporal !0
+  %ptr2 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 2
+  store <4 x float> zeroinitializer, <4 x float>* %ptr2, align 16, !nontemporal !0
+  %ptr3 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 3
+  store <4 x float> zeroinitializer, <4 x float>* %ptr3, align 16, !nontemporal !0
+  %ptr4 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 4
+  store <4 x float> zeroinitializer, <4 x float>* %ptr4, align 16, !nontemporal !0
+  %ptr5 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 5
+  store <4 x float> zeroinitializer, <4 x float>* %ptr5, align 16, !nontemporal !0
+  %ptr6 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 6
+  store <4 x float> zeroinitializer, <4 x float>* %ptr6, align 16, !nontemporal !0
+  %ptr7 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 7
+  store <4 x float> zeroinitializer, <4 x float>* %ptr7, align 16, !nontemporal !0
+  ret void
+}
+
+define void @nontemporal_stores_2(<4 x float>* nocapture %dst) {
+; CHECK-LABEL: @nontemporal_stores_2
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %dst, align 16, !nontemporal !0
+; CHECK: store <4 x float> zeroinitializer, <4 x float>* %ptr1, align 16, !nontemporal !0
+; CHECK-NEXT: ret void
+entry:
+  store <4 x float> zeroinitializer, <4 x float>* %dst, align 16, !nontemporal !0
+  %ptr1 = getelementptr inbounds <4 x float>, <4 x float>* %dst, i64 1
+  store <4 x float> zeroinitializer, <4 x float>* %ptr1, align 16, !nontemporal !0
+  ret void
+}
+
+!0 = !{i32 1}