[InstCombine] Simplify MemTransferInst's source and dest alignments separately

[dneilson]

Summary

This change is part of step five in the series of changes to remove alignment argument from
memcpy/memmove/memset in favour of alignment attributes. In particular, this changes the
InstCombine pass to cease using the deprecated MemoryIntrinsic::getAlignment() method, and
instead we use the separate getSourceAlignment and getDestAlignment APIs to simplify
the source and destination alignment attributes separately.

Steps:
Step 1) Remove alignment parameter and create alignment parameter attributes for
memcpy/memmove/memset. ( rL322965, rC322964, rL322963 )
Step 2) Expand the IRBuilder API to allow creation of memcpy/memmove with differing
source and dest alignments. ( rL323597 )
Step 3) Update Clang to use the new IRBuilder API. ( rC323617 )
Step 4) Update Polly to use the new IRBuilder API. ( rL323618 )
Step 5) Update LLVM passes that create memcpy/memmove calls to use the new IRBuilder API,
and those that use use MemIntrinsicInst::[get|set]Alignment() to use [get|set]DestAlignment()
and [get|set]SourceAlignment() instead. ( rL323886, r323891 )
Step 6) Remove the single-alignment IRBuilder API for memcpy/memmove, and the
MemIntrinsicInst::[get|set]Alignment() methods.

Reference

http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312083.html

[dneilson]

ping

[efriedma]

lib/Transforms/InstCombine/InstCombineCalls.cpp

242

Aren't SrcAlign and CopySrcAlign always the same?

[dneilson]

lib/Transforms/InstCombine/InstCombineCalls.cpp

242

I'm guessing that the original code was there because it's possible for there to be a greater alignment value on the memory intrinsic than getKnownAlignment() would give us. I'm not really sure how that would happen.... I suppose that it's possible that now that the alignment on the memory intrinsic isn't just the min of the two arg alignments that these lines are now redundant.

[efriedma]

lib/Transforms/InstCombine/InstCombineCalls.cpp

242

I'm not really sure how that would happen....

In the old world, SrcAlign could be larger than CopyAlign if the dest has low/unknown alignment. And CopyAlign could be larger than SrcAlign if the frontend or some other pass computed better alignment than getKnownAlignment.

In the new world, CopySrcAlign is always greater than or equal to SrcAlign; you explicitly check it a few lines earlier.

[dneilson]

• Was being stupid. We can just use the alignment from the memory intrinsic on the materialized loads/stores, so do that.

[efriedma]

LGTM

lib/Transforms/InstCombine/InstCombineCalls.cpp

InstCombiner::SimplifyElementUnorderedAtomicMemCpy(AtomicMemCpyInst *AMI) {
 
   // Set the number of elements of the copy to 0, it will be deleted on the
   // next iteration.
   AMI->setLength(Constant::getNullValue(LengthCI->getType()));
   return AMI;
 }
 
 Instruction *InstCombiner::SimplifyMemTransfer(MemIntrinsic *MI) {
-  unsigned DstAlign = getKnownAlignment(MI->getArgOperand(0), DL, MI, &AC, &DT);
-  unsigned SrcAlign = getKnownAlignment(MI->getArgOperand(1), DL, MI, &AC, &DT);
-  unsigned MinAlign = std::min(DstAlign, SrcAlign);
-  unsigned CopyAlign = MI->getAlignment();
-
-  // FIXME: Check & simplify source & dest alignments separately
-  if (CopyAlign < MinAlign) {
-    MI->setAlignment(MinAlign);
+  unsigned DstAlign = getKnownAlignment(MI->getRawDest(), DL, MI, &AC, &DT);
+  unsigned CopyDstAlign = MI->getDestAlignment();
+  if (CopyDstAlign < DstAlign){
+    MI->setDestAlignment(DstAlign);
+    return MI;
+  }
+
+  auto* MTI = cast<MemTransferInst>(MI);
+  unsigned SrcAlign = getKnownAlignment(MTI->getRawSource(), DL, MI, &AC, &DT);
+  unsigned CopySrcAlign = MTI->getSourceAlignment();
+  if (CopySrcAlign < SrcAlign) {
+    MTI->setSourceAlignment(SrcAlign);
     return MI;
   }
 
   // If MemCpyInst length is 1/2/4/8 bytes then replace memcpy with
   // load/store.
   ConstantInt *MemOpLength = dyn_cast<ConstantInt>(MI->getArgOperand(2));
   if (!MemOpLength) return nullptr;
 
if (M->getNumOperands() == 3 && M->getOperand(0) &&
         M->getOperand(1) &&
         mdconst::hasa<ConstantInt>(M->getOperand(1)) &&
         mdconst::extract<ConstantInt>(M->getOperand(1))->getValue() ==
         Size &&
         M->getOperand(2) && isa<MDNode>(M->getOperand(2)))
       CopyMD = cast<MDNode>(M->getOperand(2));
   }
 
-  // If the memcpy/memmove provides better alignment info than we can
-  // infer, use it.
-  SrcAlign = std::max(SrcAlign, CopyAlign);
-  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);
+  // Alignment from the mem intrinsic will be better, so use it.
+  L->setAlignment(CopySrcAlign);
   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);
+  // Alignment from the mem intrinsic will be better, so use it.
+  S->setAlignment(CopyDstAlign);
   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;

test/Transforms/InstCombine/memcpy-from-global.ll

 
 ; CHECK-LABEL: @test2(
 
 ; %A alloca is deleted
 ; CHECK-NEXT: alloca [124 x i8]
 ; CHECK-NEXT: getelementptr inbounds [124 x i8], [124 x i8]*
 
 ; use @G instead of %A
-; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 8 %{{.*}}, i8* align 8 getelementptr inbounds (%T, %T* @G, i64 0, i32 0)
+; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 8 %{{.*}}, i8* align 16 getelementptr inbounds (%T, %T* @G, i64 0, i32 0)
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %a, i8* align 4 bitcast (%T* @G to i8*), i64 124, i1 false)
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %b, i8* align 4 %a, i64 124, i1 false)
   call void @bar(i8* %b)
   ret void
 }
 
 define void @test2_addrspacecast() {
   %A = alloca %T

test/Transforms/InstCombine/memmove.ll

define void @test1(i8* %A, i8* %B, i32 %N) {
   ;; CHECK-NEXT: ret void
   call void @llvm.memmove.p0i8.p0i8.i32(i8* %A, i8* %B, i32 0, i1 false)
   ret void
 }
 
 define void @test2(i8* %A, i32 %N) {
   ;; dest can't alias source since we can't write to source!
   ;; CHECK-LABEL: test2
-  ;; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 %A, i8* align 1 getelementptr inbounds ([33 x i8], [33 x i8]* @S, i{{32|64}} 0, i{{32|64}} 0), i32 %N, i1 false)
+  ;; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 %A, i8* align 16 getelementptr inbounds ([33 x i8], [33 x i8]* @S, i{{32|64}} 0, i{{32|64}} 0), i32 %N, i1 false)
   ;; CHECK-NEXT: ret void
   call void @llvm.memmove.p0i8.p0i8.i32(i8* %A, i8* getelementptr inbounds ([33 x i8], [33 x i8]* @S, i32 0, i32 0), i32 %N, i1 false)
   ret void
 }
 
 define i32 @test3([1024 x i8]* %target) { ; arg: [1024 x i8]*> [#uses=1]
   ;; CHECK-LABEL: test3
   ;; CHECK-NEXT: [[P1:%[^\s]+]] = bitcast [1024 x i8]* %target to i16*