[InstCombine] Simplify MemTransferInst with type inference

[gandhi21299]

Summary

Use pointee type of a MemTransferInst's source value,
instead of an integer type, when replacing it with
a load/store pair.

[gandhi21299]

• Simplified @test7 in memcpy_alloca.ll

[arsenm]

llvm/test/Transforms/InstCombine/memcpy_alloca.ll

83

Why is the cast necessary for the test?

[gandhi21299]

llvm/test/Transforms/InstCombine/memcpy_alloca.ll

83

InstCombine reduces the function down to ret void when I remove the cast.

[gandhi21299]

• Simplified test further by eliminating addrspacecast

[nikic]

There are regressions in tests. To avoid them you probably need to restrict this to the case where the alloca type is a simple type rather than an aggregate. Currently you end up introducing a load of [2 x i32] type in the swap example, which is something we're not good at handling.

Could you explain what the larger context for this change is, possibly with a phase ordering test? We will not be able to reliably "guess" the type here, and I'm not sure special-casing the alloca case makes sense. The general expectation is that if the chosen type is "incorrect", we'll optimize away introduced bitcasts when that becomes evident.

The long term plan here is that this will make use of the byte type, because other choices are generally incorrect due to either poison-propagation or provenance issues. The introduction of the byte type is somewhat stuck right now though.

[nikic]

Also, there are a number of correctness issues in your implementation: a) You don't check that the alloca type has the same size as the memcpy, so you might end up copying e.g. more bytes here. b) The alloca type might have interior padding. E.g. a load/store of { i8, i32 } is not the same as a memcpy of 8 bytes.

[nlopes]

Agreed with @nikic, this patch can't be right. The type of the alloca is not meaningful. Alas, we could replace it with a single bytes parameter in the future.
Lowering memcpys to load/store is tricky business. Let's not add more such optimizations until there's a proper solution implemented.

[gandhi21299]

@nikic @nlopes Is there a way to reliably infer the type of alloca then? The purpose is to enable certain optimizations like merging multiple stores into a phi, etc.

[nikic]

In D150595#4346984, @gandhi21299 wrote:

@nikic @nlopes Is there a way to reliably infer the type of alloca then? The purpose is to enable certain optimizations like merging multiple stores into a phi, etc.

The general way to handle this is to support mismatched types in relevant optimizations by inserting bitcasts. See D147348 for an example of how to do that.

[gandhi21299]

Thanks everyone for feedback to this review. I implemented a different approach in D150900, which is rather safer than this approach. I will abandon this revision as a result.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

if (*CopyDstAlign < Size || *CopySrcAlign < Size)
       return nullptr;
 
   // Use an integer load+store unless we can find something better.
   unsigned SrcAddrSp =
     cast<PointerType>(MI->getArgOperand(1)->getType())->getAddressSpace();
   unsigned DstAddrSp =
     cast<PointerType>(MI->getArgOperand(0)->getType())->getAddressSpace();
 
-  IntegerType* IntType = IntegerType::get(MI->getContext(), Size<<3);
-  Type *NewSrcPtrTy = PointerType::get(IntType, SrcAddrSp);
-  Type *NewDstPtrTy = PointerType::get(IntType, DstAddrSp);
+  auto *AI = dyn_cast<AllocaInst>(MI->getSource());
+  auto *SrcTy = AI ? AI->getAllocatedType()
+                   : IntegerType::get(MI->getContext(), Size << 3);
+  Type *NewSrcPtrTy = PointerType::get(SrcTy, SrcAddrSp);
+  Type *NewDstPtrTy = PointerType::get(SrcTy, DstAddrSp);
 
   // If the memcpy has metadata describing the members, see if we can get the
   // TBAA tag describing our copy.
   MDNode *CopyMD = nullptr;
   if (MDNode *M = MI->getMetadata(LLVMContext::MD_tbaa)) {
     CopyMD = M;
   } else if (MDNode *M = MI->getMetadata(LLVMContext::MD_tbaa_struct)) {
     if (M->getNumOperands() == 3 && M->getOperand(0) &&
         mdconst::hasa<ConstantInt>(M->getOperand(0)) &&
         mdconst::extract<ConstantInt>(M->getOperand(0))->isZero() &&
         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));
   }
 
   Value *Src = Builder.CreateBitCast(MI->getArgOperand(1), NewSrcPtrTy);
   Value *Dest = Builder.CreateBitCast(MI->getArgOperand(0), NewDstPtrTy);
-  LoadInst *L = Builder.CreateLoad(IntType, Src);
+  LoadInst *L = Builder.CreateLoad(SrcTy, Src);
   // 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);

llvm/test/Transforms/InstCombine/memcpy_alloca.ll

 ;
   %a = alloca [7 x i8]
   %p2 = getelementptr i16, ptr %a, i32 1
   store i16 42, ptr %p2
   call void @llvm.memcpy.p0.p0.i64(ptr %dest, ptr %p2, i64 7, i1 false)
   ret void
 }
 
+define ptr @test7(ptr %dest) {
+; CHECK-LABEL: @test7(
+; CHECK-NEXT:    [[ALLOCA:%.*]] = alloca half, align 2
+; CHECK-NEXT:    [[TMP1:%.*]] = load half, ptr [[ALLOCA]], align 2
+; CHECK-NEXT:    store half [[TMP1]], ptr [[DEST:%.*]], align 1
+; CHECK-NEXT:    ret ptr [[ALLOCA]]
+;
+  %alloca = alloca half, align 2
+  call void @llvm.memcpy.p0.p0.i64(ptr %dest, ptr %alloca, i64 2, i1 false)
+  ret ptr %alloca
+}
+
 declare void @llvm.memcpy.p0.p0.i64(ptr, ptr, i64, i1)

llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling-2.ll

 
 ; Function Attrs: mustprogress nounwind uwtable
 define dso_local void @foo(i32 noundef %arg, ptr noundef nonnull align 4 dereferenceable(8) %arg1) #0 {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:  bb:
 ; CHECK-NEXT:    [[I9:%.*]] = sdiv i32 [[ARG:%.*]], 128
 ; CHECK-NEXT:    [[I10:%.*]] = shl nsw i32 [[I9]], 7
 ; CHECK-NEXT:    [[ARG_OFF:%.*]] = add i32 [[ARG]], 127
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp ult i32 [[ARG_OFF]], 255
-; CHECK-NEXT:    br i1 [[TMP0]], label [[BB12:%.*]], label [[BB13:%.*]]
+; CHECK-NEXT:    [[I11_NOT9:%.*]] = icmp ult i32 [[ARG_OFF]], 255
+; CHECK-NEXT:    br i1 [[I11_NOT9]], label [[BB12:%.*]], label [[BB13:%.*]]
 ; CHECK:       bb12.loopexit:
-; CHECK-NEXT:    [[I3_SROA_8_0_INSERT_EXT:%.*]] = zext i32 [[I21_3:%.*]] to i64
-; CHECK-NEXT:    [[I3_SROA_8_0_INSERT_SHIFT:%.*]] = shl nuw i64 [[I3_SROA_8_0_INSERT_EXT]], 32
-; CHECK-NEXT:    [[I3_SROA_0_0_INSERT_EXT:%.*]] = zext i32 [[I21_2:%.*]] to i64
-; CHECK-NEXT:    [[I3_SROA_0_0_INSERT_INSERT:%.*]] = or i64 [[I3_SROA_8_0_INSERT_SHIFT]], [[I3_SROA_0_0_INSERT_EXT]]
+; CHECK-NEXT:    [[TMP0:%.*]] = bitcast i64 [[I3_SROA_0_4_INSERT_INSERT32:%.*]] to <2 x i32>
 ; CHECK-NEXT:    br label [[BB12]]
 ; CHECK:       bb12:
-; CHECK-NEXT:    [[TMP1:%.*]] = phi i64 [ [[I3_SROA_0_0_INSERT_INSERT]], [[BB12_LOOPEXIT:%.*]] ], [ 180388626456, [[BB:%.*]] ]
-; CHECK-NEXT:    store i64 [[TMP1]], ptr [[ARG1:%.*]], align 4, !tbaa [[TBAA5:![0-9]+]]
+; CHECK-NEXT:    [[TMP1:%.*]] = phi <2 x i32> [ [[TMP0]], [[BB12_LOOPEXIT:%.*]] ], [ <i32 24, i32 42>, [[BB:%.*]] ]
+; CHECK-NEXT:    store <2 x i32> [[TMP1]], ptr [[ARG1:%.*]], align 4, !tbaa [[TBAA5:![0-9]+]]
 ; CHECK-NEXT:    ret void
 ; CHECK:       bb13:
-; CHECK-NEXT:    [[I3_SROA_8_0:%.*]] = phi i32 [ [[I21_3]], [[BB13]] ], [ 42, [[BB]] ]
-; CHECK-NEXT:    [[I3_SROA_0_0:%.*]] = phi i32 [ [[I21_2]], [[BB13]] ], [ 24, [[BB]] ]
-; CHECK-NEXT:    [[I4_05:%.*]] = phi i32 [ [[I24_3:%.*]], [[BB13]] ], [ 0, [[BB]] ]
-; CHECK-NEXT:    [[I21:%.*]] = mul nsw i32 [[I3_SROA_0_0]], [[I4_05]]
-; CHECK-NEXT:    [[I24:%.*]] = or i32 [[I4_05]], 1
-; CHECK-NEXT:    [[I21_1:%.*]] = mul nsw i32 [[I3_SROA_8_0]], [[I24]]
-; CHECK-NEXT:    [[I24_1:%.*]] = or i32 [[I4_05]], 2
-; CHECK-NEXT:    [[I21_2]] = mul nsw i32 [[I21]], [[I24_1]]
-; CHECK-NEXT:    [[I24_2:%.*]] = or i32 [[I4_05]], 3
-; CHECK-NEXT:    [[I21_3]] = mul nsw i32 [[I21_1]], [[I24_2]]
-; CHECK-NEXT:    [[I24_3]] = add nuw nsw i32 [[I4_05]], 4
+; CHECK-NEXT:    [[I3_SROA_0_0:%.*]] = phi i64 [ [[I3_SROA_0_4_INSERT_INSERT32]], [[BB13]] ], [ 180388626456, [[BB]] ]
+; CHECK-NEXT:    [[I4_010:%.*]] = phi i32 [ [[I24_3:%.*]], [[BB13]] ], [ 0, [[BB]] ]
+; CHECK-NEXT:    [[I3_SROA_0_0_EXTRACT_TRUNC:%.*]] = trunc i64 [[I3_SROA_0_0]] to i32
+; CHECK-NEXT:    [[I21:%.*]] = mul nsw i32 [[I4_010]], [[I3_SROA_0_0_EXTRACT_TRUNC]]
+; CHECK-NEXT:    [[I24:%.*]] = or i32 [[I4_010]], 1
+; CHECK-NEXT:    [[I3_SROA_0_4_EXTRACT_SHIFT:%.*]] = lshr i64 [[I3_SROA_0_0]], 32
+; CHECK-NEXT:    [[I3_SROA_0_4_EXTRACT_TRUNC:%.*]] = trunc i64 [[I3_SROA_0_4_EXTRACT_SHIFT]] to i32
+; CHECK-NEXT:    [[I21_1:%.*]] = mul nsw i32 [[I24]], [[I3_SROA_0_4_EXTRACT_TRUNC]]
+; CHECK-NEXT:    [[I24_1:%.*]] = or i32 [[I4_010]], 2
+; CHECK-NEXT:    [[I21_2:%.*]] = mul nsw i32 [[I21]], [[I24_1]]
+; CHECK-NEXT:    [[I3_SROA_0_0_INSERT_EXT20:%.*]] = zext i32 [[I21_2]] to i64
+; CHECK-NEXT:    [[I24_2:%.*]] = or i32 [[I4_010]], 3
+; CHECK-NEXT:    [[I21_3:%.*]] = mul nsw i32 [[I21_1]], [[I24_2]]
+; CHECK-NEXT:    [[I3_SROA_0_4_INSERT_EXT29:%.*]] = zext i32 [[I21_3]] to i64
+; CHECK-NEXT:    [[I3_SROA_0_4_INSERT_SHIFT30:%.*]] = shl nuw i64 [[I3_SROA_0_4_INSERT_EXT29]], 32
+; CHECK-NEXT:    [[I3_SROA_0_4_INSERT_INSERT32]] = or i64 [[I3_SROA_0_4_INSERT_SHIFT30]], [[I3_SROA_0_0_INSERT_EXT20]]
+; CHECK-NEXT:    [[I24_3]] = add nuw nsw i32 [[I4_010]], 4
 ; CHECK-NEXT:    [[I11_NOT_3:%.*]] = icmp eq i32 [[I24_3]], [[I10]]
 ; CHECK-NEXT:    br i1 [[I11_NOT_3]], label [[BB12_LOOPEXIT]], label [[BB13]], !llvm.loop [[LOOP8:![0-9]+]]
 ;
 bb:
   %i = alloca i32, align 4
   %i2 = alloca ptr, align 8
   %i3 = alloca %t0, align 4
   %i4 = alloca i32, align 4

llvm/test/Transforms/PhaseOrdering/swap-promotion.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -O1 < %s | FileCheck %s
 ; RUN: opt -S -O2 < %s | FileCheck %s
 ; RUN: opt -S -O3 < %s | FileCheck %s
 
 define void @swap(ptr %p1, ptr %p2) {
 ; CHECK-LABEL: @swap(
 ; CHECK-NEXT:    [[TMP1:%.*]] = load i64, ptr [[P1:%.*]], align 1
+; CHECK-NEXT:    [[TMP_SROA_0_0_EXTRACT_TRUNC:%.*]] = trunc i64 [[TMP1]] to i32
+; CHECK-NEXT:    [[TMP_SROA_2_0_EXTRACT_SHIFT:%.*]] = lshr i64 [[TMP1]], 32
+; CHECK-NEXT:    [[TMP_SROA_2_0_EXTRACT_TRUNC:%.*]] = trunc i64 [[TMP_SROA_2_0_EXTRACT_SHIFT]] to i32
 ; CHECK-NEXT:    [[TMP2:%.*]] = load i64, ptr [[P2:%.*]], align 1
 ; CHECK-NEXT:    store i64 [[TMP2]], ptr [[P1]], align 1
-; CHECK-NEXT:    store i64 [[TMP1]], ptr [[P2]], align 1
+; CHECK-NEXT:    store i32 [[TMP_SROA_0_0_EXTRACT_TRUNC]], ptr [[P2]], align 1
+; CHECK-NEXT:    [[P2_REPACK3:%.*]] = getelementptr inbounds [2 x i32], ptr [[P2]], i64 0, i64 1
+; CHECK-NEXT:    store i32 [[TMP_SROA_2_0_EXTRACT_TRUNC]], ptr [[P2_REPACK3]], align 1
 ; CHECK-NEXT:    ret void
 ;
   %tmp = alloca [2 x i32]
   call void @llvm.memcpy.p0.p0.i64(ptr %tmp, ptr %p1, i64 8, i1 false)
   call void @llvm.memcpy.p0.p0.i64(ptr %p1, ptr %p2, i64 8, i1 false)
   call void @llvm.memcpy.p0.p0.i64(ptr %p2, ptr %tmp, i64 8, i1 false)
   ret void
 }