[SROA] Preserve llvm.mem.parallel_loop_access metadata

[dorit]

Summary

SROA doesn't preserve the llvm.mem.parallel_loop_access metadata when it transforms loads/stores.

(This patch is not a systematic effort to catch all the cases missed by SROA, but it at least covers those exposed by this testcase / by PR28981).

[hfinkel]

SROA doesn't preserve the llvm.mem.parallel_loop_access metadata when it transforms loads/stores.
(This patch is not a systematic effort to catch all the cases missed by SROA, but it at least covers those exposed by this testcase / by PR28981).

The higher-level questions here clearly seem to be:

1. Does SROA every do anything that will add loop-carried dependencies? If not, we could do this everywhere.
2. Does SROA also drop AA metadata we should be preserving in the same circumstances? I only see metadata handling in speculatePHINodeLoads and speculateSelectInstLoads, but nowhere else.

[chandlerc]

In D23549#527261, @hfinkel wrote:

SROA doesn't preserve the llvm.mem.parallel_loop_access metadata when it transforms loads/stores.
(This patch is not a systematic effort to catch all the cases missed by SROA, but it at least covers those exposed by this testcase / by PR28981).

The higher-level questions here clearly seem to be:

1. Does SROA every do anything that will add loop-carried dependencies? If not, we could do this everywhere.

Off the top of my head I can't figure out how it could do this anywhere other than the PHI speculation parts.

2. Does SROA also drop AA metadata we should be preserving in the same circumstances? I only see metadata handling in speculatePHINodeLoads and speculateSelectInstLoads, but nowhere else.

I can absolutely believe we're dropping metadata when we rewrite stuff but fail to promote it to registers. I think the rewriting logic here never got the most careful of analysis because we "usually" think of it as rewriting in preparation for promotion, but we definitely have lots of rewrites at this point that don't involve in promotion so we should probably systematically go through this.

[dorit]

Slightly updated the testcase: Moved the RUN command to the top of the file; added CHECK and CHECK-NOTs to verify not only that stores without metadata don't appear before the first positive match, but that they also don't appear between and after the positive matches.

[dorit]

In D23549#527278, @chandlerc wrote:

...

I can absolutely believe we're dropping metadata when we rewrite stuff but fail to promote it to registers. I think the rewriting logic here never got the most careful of analysis because we "usually" think of it as rewriting in preparation for promotion, but we definitely have lots of rewrites at this point that don't involve in promotion so we should probably systematically go through this.

Agreed... Is it ok though to commit this patch, as a tiny step towards that (which allows the mem.parallel tags reach the vectorizer) ?

[dorit]

ping?

[dorit]

ping^2...

[delena]

llvm/lib/Transforms/Scalar/SROA.cpp

2550

Please use copyMetadata() instead.

[dorit]

Replaced get/setMetadata with copyMetadata as per Elena's comment.

llvm/lib/Transforms/Scalar/SROA.cpp

if (DL.getTypeSizeInBits(V->getType()) != IntTy->getBitWidth()) {
           IRB.CreateAlignedLoad(&NewAI, NewAI.getAlignment(), "oldload");
       Old = convertValue(DL, IRB, Old, IntTy);
       assert(BeginOffset >= NewAllocaBeginOffset && "Out of bounds offset");
       uint64_t Offset = BeginOffset - NewAllocaBeginOffset;
       V = insertInteger(DL, IRB, Old, SI.getValueOperand(), Offset, "insert");
     }
     V = convertValue(DL, IRB, V, NewAllocaTy);
     StoreInst *Store = IRB.CreateAlignedStore(V, &NewAI, NewAI.getAlignment());
+    auto LoopParMD =
+      makeArrayRef((unsigned)LLVMContext::MD_mem_parallel_loop_access);
+    Store->copyMetadata(SI, LoopParMD);
     Pass.DeadInsts.insert(&SI);
-    (void)Store;
     DEBUG(dbgs() << "          to: " << *Store << "\n");
     return true;
   }
 
   bool visitStoreInst(StoreInst &SI) {
     DEBUG(dbgs() << "    original: " << SI << "\n");
     Value *OldOp = SI.getOperand(1);
     assert(OldOp == OldPtr);
if (NewBeginOffset == NewAllocaBeginOffset &&
       V = convertValue(DL, IRB, V, NewAllocaTy);
       NewSI = IRB.CreateAlignedStore(V, &NewAI, NewAI.getAlignment(),
                                      SI.isVolatile());
     } else {
       Value *NewPtr = getNewAllocaSlicePtr(IRB, V->getType()->getPointerTo());
       NewSI = IRB.CreateAlignedStore(V, NewPtr, getSliceAlign(V->getType()),
                                      SI.isVolatile());
     }
+    auto LoopParMD =
+      makeArrayRef((unsigned)LLVMContext::MD_mem_parallel_loop_access);
+    NewSI->copyMetadata(SI, LoopParMD);
     if (SI.isVolatile())
       NewSI->setAtomic(SI.getOrdering(), SI.getSynchScope());
     Pass.DeadInsts.insert(&SI);
     deleteIfTriviallyDead(OldOp);
 
     DEBUG(dbgs() << "          to: " << *NewSI << "\n");
     return NewSI->getPointerOperand() == &NewAI && !SI.isVolatile();
   }
for (LoadInst *LI : Loads) {
 
     Instruction *BasePtr = cast<Instruction>(LI->getPointerOperand());
     IRB.SetInsertPoint(LI);
 
     DEBUG(dbgs() << "  Splitting load: " << *LI << "\n");
 
     uint64_t PartOffset = 0, PartSize = Offsets.Splits.front();
     int Idx = 0, Size = Offsets.Splits.size();
+    auto LoopParMD =
+      makeArrayRef((unsigned)LLVMContext::MD_mem_parallel_loop_access);
     for (;;) {
       auto *PartTy = Type::getIntNTy(Ty->getContext(), PartSize * 8);
       auto *PartPtrTy = PartTy->getPointerTo(LI->getPointerAddressSpace());
       LoadInst *PLoad = IRB.CreateAlignedLoad(
           getAdjustedPtr(IRB, DL, BasePtr,
                          APInt(DL.getPointerSizeInBits(), PartOffset),
                          PartPtrTy, BasePtr->getName() + "."),
           getAdjustedAlignment(LI, PartOffset, DL), /*IsVolatile*/ false,
           LI->getName());
+      PLoad->copyMetadata(*LI, LoopParMD); 
 
       // Append this load onto the list of split loads so we can find it later
       // to rewrite the stores.
       SplitLoads.push_back(PLoad);
 
       // Now build a new slice for the alloca.
       NewSlices.push_back(
           Slice(BaseOffset + PartOffset, BaseOffset + PartOffset + PartSize,
for (User *LU : LI->users()) {
         auto *PartPtrTy =
             PLoad->getType()->getPointerTo(SI->getPointerAddressSpace());
 
         StoreInst *PStore = IRB.CreateAlignedStore(
             PLoad, getAdjustedPtr(IRB, DL, StoreBasePtr,
                                   APInt(DL.getPointerSizeInBits(), PartOffset),
                                   PartPtrTy, StoreBasePtr->getName() + "."),
             getAdjustedAlignment(SI, PartOffset, DL), /*IsVolatile*/ false);
-        (void)PStore;
+        PStore->copyMetadata(*LI, LoopParMD);
         DEBUG(dbgs() << "      +" << PartOffset << ":" << *PStore << "\n");
       }
 
       // We want to immediately iterate on any allocas impacted by splitting
       // this store, and we have to track any promotable alloca (indicated by
       // a direct store) as needing to be resplit because it is no longer
       // promotable.
       if (AllocaInst *OtherAI = dyn_cast<AllocaInst>(StoreBasePtr)) {

llvm/test/Transforms/SROA/mem-par-metadata-sroa.ll

+; RUN: opt < %s -sroa -S | FileCheck %s
+;
+; Make sure the llvm.mem.parallel_loop_access meta-data is preserved
+; when a load/store is replaced with another load/store by sroa
+;
+; class Complex {
+; private:
+;  float real_;
+;  float imaginary_;
+;
+; public:
+;   Complex() : real_(0), imaginary_(0) { }
+;   Complex(float real, float imaginary) : real_(real), imaginary_(imaginary) { }
+;   Complex(const Complex &rhs) : real_(rhs.real()), imaginary_(rhs.imaginary()) { }
+; 
+;   inline float real() const { return real_; }
+;   inline float imaginary() const { return imaginary_; }
+; 
+;   Complex operator+(const Complex& rhs) const
+;   {
+;     return Complex(real_ + rhs.real_, imaginary_ + rhs.imaginary_);
+;   }
+; };
+; 
+; void test(Complex *out, long size)
+; {
+;     #pragma clang loop vectorize(assume_safety)
+;     for (long offset = 0; offset < size; ++offset) {
+;       Complex t0 = out[offset];
+;       out[offset] = t0 + t0;
+;     }
+; }
+
+; CHECK: for.body:
+; CHECK-NOT:  store i32 %{{.*}}, i32* %{{.*}}, align 4
+; CHECK: store i32 %{{.*}}, i32* %{{.*}}, align 4, !llvm.mem.parallel_loop_access !1
+; CHECK-NOT:  store i32 %{{.*}}, i32* %{{.*}}, align 4
+; CHECK: store i32 %{{.*}}, i32* %{{.*}}, align 4, !llvm.mem.parallel_loop_access !1
+; CHECK-NOT:  store i32 %{{.*}}, i32* %{{.*}}, align 4
+; CHECK: br label
+
+; ModuleID = '<stdin>'
+source_filename = "mem-par-metadata-sroa1.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%class.Complex = type { float, float }
+
+; Function Attrs: norecurse nounwind uwtable
+define void @_Z4testP7Complexl(%class.Complex* nocapture %out, i64 %size) local_unnamed_addr #0 {
+entry:
+  %t0 = alloca %class.Complex, align 4
+  %ref.tmp = alloca i64, align 8
+  %tmpcast = bitcast i64* %ref.tmp to %class.Complex*
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.body, %entry
+  %offset.0 = phi i64 [ 0, %entry ], [ %inc, %for.body ]
+  %cmp = icmp slt i64 %offset.0, %size
+  br i1 %cmp, label %for.body, label %for.end
+
+for.body:                                         ; preds = %for.cond
+  %arrayidx = getelementptr inbounds %class.Complex, %class.Complex* %out, i64 %offset.0
+  %real_.i = getelementptr inbounds %class.Complex, %class.Complex* %t0, i64 0, i32 0
+  %real_.i.i = getelementptr inbounds %class.Complex, %class.Complex* %arrayidx, i64 0, i32 0
+  %0 = load float, float* %real_.i.i, align 4, !llvm.mem.parallel_loop_access !1
+  store float %0, float* %real_.i, align 4, !llvm.mem.parallel_loop_access !1
+  %imaginary_.i = getelementptr inbounds %class.Complex, %class.Complex* %t0, i64 0, i32 1
+  %imaginary_.i.i = getelementptr inbounds %class.Complex, %class.Complex* %arrayidx, i64 0, i32 1
+  %1 = load float, float* %imaginary_.i.i, align 4, !llvm.mem.parallel_loop_access !1
+  store float %1, float* %imaginary_.i, align 4, !llvm.mem.parallel_loop_access !1
+  %arrayidx1 = getelementptr inbounds %class.Complex, %class.Complex* %out, i64 %offset.0
+  %real_.i1 = getelementptr inbounds %class.Complex, %class.Complex* %t0, i64 0, i32 0
+  %2 = load float, float* %real_.i1, align 4, !noalias !3, !llvm.mem.parallel_loop_access !1
+  %real_2.i = getelementptr inbounds %class.Complex, %class.Complex* %t0, i64 0, i32 0
+  %3 = load float, float* %real_2.i, align 4, !noalias !3, !llvm.mem.parallel_loop_access !1
+  %add.i = fadd float %2, %3
+  %imaginary_.i2 = getelementptr inbounds %class.Complex, %class.Complex* %t0, i64 0, i32 1
+  %4 = load float, float* %imaginary_.i2, align 4, !noalias !3, !llvm.mem.parallel_loop_access !1
+  %imaginary_3.i = getelementptr inbounds %class.Complex, %class.Complex* %t0, i64 0, i32 1
+  %5 = load float, float* %imaginary_3.i, align 4, !noalias !3, !llvm.mem.parallel_loop_access !1
+  %add4.i = fadd float %4, %5
+  %real_.i.i3 = getelementptr inbounds %class.Complex, %class.Complex* %tmpcast, i64 0, i32 0
+  store float %add.i, float* %real_.i.i3, align 4, !alias.scope !3, !llvm.mem.parallel_loop_access !1
+  %imaginary_.i.i4 = getelementptr inbounds %class.Complex, %class.Complex* %tmpcast, i64 0, i32 1
+  store float %add4.i, float* %imaginary_.i.i4, align 4, !alias.scope !3, !llvm.mem.parallel_loop_access !1
+  %6 = bitcast %class.Complex* %arrayidx1 to i64*
+  %7 = load i64, i64* %ref.tmp, align 8, !llvm.mem.parallel_loop_access !1
+  store i64 %7, i64* %6, align 4, !llvm.mem.parallel_loop_access !1
+  %inc = add nsw i64 %offset.0, 1
+  br label %for.cond, !llvm.loop !1
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1) #1
+
+attributes #0 = { norecurse nounwind uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 4.0.0 (cfe/trunk 277751)"}
+!1 = distinct !{!1, !2}
+!2 = !{!"llvm.loop.vectorize.enable", i1 true}
+!3 = !{!4}
+!4 = distinct !{!4, !5, !"_ZNK7ComplexplERKS_: %agg.result"}
+!5 = distinct !{!5, !"_ZNK7ComplexplERKS_"}