[InstCombine] Preserve llvm.mem.parallel_loop_access metadata when replacing memcpy with ld/st

[dorit]

Summary

When InstCombine replaces a memcpy with loads+stores it does not copy over the llvm.mem.parallel_loop_access from the memcpy instruction.

BTW, currently our memcpy's don't get annotated with llvm.mem.parallel_loop_access by clang (just opened a PR for this one: 28980). So this scenario is not yet reproducible automatically (need to add the llvm.mem.parallel_loop_access manually to the memcpy in the .ll).

[Ayal]

llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll

2

Can add here that this is related to PR28981, for the broader context.

[mkuper]

llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll

13

Can you move the RUN line to be the first line in the file, before the docs?

15

You're missing a ":" at the end of your CHECKs and CHECK-NOTs.
But I think if you actually add them, the test will fail, since the CHECK-NOT will match the beginning of the longer line. You'll need to rewrite the pattern to match a full line (adding a {{$}} should work), but even then, I'm not entirely sure this will do the right thing.

[mkuper]

llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll

15

On second thought, the CHECK-NOTs may work fine as-is - although I'm still somewhat suspicious. :-)
In any case, the ":" are still missing.

[dorit]

Addressed Michael's comments about the testcase: moved RUN line to top of file ; added missing ":" after CHECK ; dropped the CHECK-NOTs, since in this specific testcase there will be no additional loads/stores beyond the one's that the positive CHECK catches, so the CHECK-NOTs are redundant.

[mkuper]

LGTM

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Instruction *InstCombiner::SimplifyMemTransfer(MemIntrinsic *MI) {
   DstAlign = std::max(DstAlign, CopyAlign);
 
   Value *Src = Builder->CreateBitCast(MI->getArgOperand(1), NewSrcPtrTy);
   Value *Dest = Builder->CreateBitCast(MI->getArgOperand(0), NewDstPtrTy);
   LoadInst *L = Builder->CreateLoad(Src, MI->isVolatile());
   L->setAlignment(SrcAlign);
   if (CopyMD)
     L->setMetadata(LLVMContext::MD_tbaa, CopyMD);
+  MDNode *LoopMemParallelMD = 
+    MI->getMetadata(LLVMContext::MD_mem_parallel_loop_access);
+  if (LoopMemParallelMD)
+    L->setMetadata(LLVMContext::MD_mem_parallel_loop_access, LoopMemParallelMD);
+
   StoreInst *S = Builder->CreateStore(L, Dest, MI->isVolatile());
   S->setAlignment(DstAlign);
   if (CopyMD)
     S->setMetadata(LLVMContext::MD_tbaa, CopyMD);
+  if (LoopMemParallelMD)
+    S->setMetadata(LLVMContext::MD_mem_parallel_loop_access, LoopMemParallelMD);
 
   // Set the size of the copy to 0, it will be deleted on the next iteration.
   MI->setArgOperand(2, Constant::getNullValue(MemOpLength->getType()));
   return MI;
 }
 
 Instruction *InstCombiner::SimplifyMemSet(MemSetInst *MI) {
   unsigned Alignment = getKnownAlignment(MI->getDest(), DL, MI, AC, DT);

llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll

+; Make sure the llvm.mem.parallel_loop_access meta-data is preserved
+; when a memcpy is replaced with a load+store by instcombine
+;
+; #include <string.h>
+; void test(char* out, long size)
+; {
+;     #pragma clang loop vectorize(assume_safety)
+;     for (long i = 0; i < size; i+=2) {
+;         memcpy(&(out[i]), &(out[i+size]), 2);
+;     }
+; }
+
+; RUN: opt < %s -instcombine -S | FileCheck %s
+; CHECK: for.body:
+; CHECK-NOT  %{{.*}} = load i16, i16* %{{.*}}, align 1
+; CHECK  %{{.*}} = load i16, i16* %{{.*}}, align 1, !llvm.mem.parallel_loop_access !1
+; CHECK-NOT  store i16 %{{.*}}, i16* %{{.*}}, align 1
+; CHECK  store i16 %{{.*}}, i16* %{{.*}}, align 1, !llvm.mem.parallel_loop_access !1
+
+
+; ModuleID = '<stdin>'
+source_filename = "memcpy.pragma.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define void @_Z4testPcl(i8* %out, i64 %size) #0 {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc, %entry
+  %i.0 = phi i64 [ 0, %entry ], [ %add2, %for.inc ]
+  %cmp = icmp slt i64 %i.0, %size
+  br i1 %cmp, label %for.body, label %for.end
+
+for.body:                                         ; preds = %for.cond
+  %arrayidx = getelementptr inbounds i8, i8* %out, i64 %i.0
+  %add = add nsw i64 %i.0, %size
+  %arrayidx1 = getelementptr inbounds i8, i8* %out, i64 %add
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %arrayidx, i8* %arrayidx1, i64 2, i32 1, i1 false), !llvm.mem.parallel_loop_access !1
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.body
+  %add2 = add nsw i64 %i.0, 2
+  br label %for.cond, !llvm.loop !2
+
+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 = { 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, !3}
+!2 = distinct !{!2, !3}
+!3 = !{!"llvm.loop.vectorize.enable", i1 true}