[Transforms] Propagate new-format TBAA tags on simplification of memory-transfer intrinsics

[kosarev]

Summary

With this patch in place, when a new-format TBAA tag is available for a memory-transfer intrinsic call, we prefer propagating that new-format tag. Otherwise, we fallback to the old approach where we try to construct a proper TBAA access tag from 'tbaa.struct' metadata.

[kosarev]

Ping.

[kosarev]

Ping.

[rjmccall]

This seems reasonable, but I don't generally review LLVM patches.

[kosarev]

Sure, I add you to reviewers of LLVM patches related the TBAA changes to make sure you are aware of the progress in this part of the work. If adding you to subscribers is preferable, I think that would work too. Just let me know. Thanks.

[kosarev]

Ping.

[kosarev]

Ping.

[sepavloff]

I applied this patch (target: i686-pc-windows-msvc) and run the test. No TBAA decorations present in the output. When running struct-assign-tbaa.ll from the same directory, the TBAA decorations are observed.

test/Transforms/InstCombine/struct-assign-tbaa-new.ll

43

Pointer size depends on the platform. I would recommend you to add a target layout. See struct-assign-tbaa.ll for example.

[kosarev]

This is due to new conventions for memory-transferring intrinsics introduced in rL322965. Will update in a moment. Thanks for reviewing.

[kosarev]

• Intrinsics calls updated to match the new conventions.
• Added the target data layout string in the test.

[sepavloff]

TBAA decorations are still absent in the case of struct-assign-tbaa-new.ll, but it does not caused by this patch. The module returned by parseIRFile does not contain TBAA metadata in the case of this test, while struct-assign-tbaa.ll works as expected. Looks like an issue of loading new TBAA data from .ll file.

[kosarev]

Hmm, somehow it works for me on r325320/Linux. Will re-check things and let you know then.

[kosarev]

Still can't reproduce the problem. Just tried opt r325330 built on Windows 8.1 with MSVS Community 2017, and I can see the metadata in the output.

[sepavloff]

I used Visual Studio 2015 14.0.25425.01 Update 3. With it TBAA decorations do not appear in opt output. On the other host I installed a fresh copy of Visual Studio 2015, it has version 14.0.25431.01 Update 3. On that host the metadata appears and test passes.

[sepavloff]

LGTM

As a possible further enhancement. memcpy accesses two memory locations, source and destination, so it is both a load and a store. Does it make sense decorate load access also?

[kosarev]

Separate tags for reading and writing aggregate accesses would certainly make sense. But that change would require some significant amount of time while being beyond the scope of the current work. This was briefly discussed before in D41539#966289.

lib/Transforms/InstCombine/InstCombineCalls.cpp

Instruction *InstCombiner::SimplifyMemTransfer(MemIntrinsic *MI) {
 
   IntegerType* IntType = IntegerType::get(MI->getContext(), Size<<3);
   Type *NewSrcPtrTy = PointerType::get(IntType, SrcAddrSp);
   Type *NewDstPtrTy = PointerType::get(IntType, 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_struct)) {
+  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)))

test/Transforms/InstCombine/struct-assign-tbaa-new.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+;
+; Verify that instcombine preserves TBAA tags when converting a memcpy into
+; a scalar load and store.
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture, i64, i1) nounwind
+
+%A = type { float }
+
+define void @test1(%A* %a1, %A* %a2) {
+entry:
+; CHECK-LABEL: @test1
+; CHECK: %[[LOAD:.*]] = load i32, {{.*}}, !tbaa [[TAG_A:!.*]]
+; CHECK: store i32 %[[LOAD]], {{.*}}, !tbaa [[TAG_A]]
+; CHECK: ret
+  %0 = bitcast %A* %a1 to i8*
+  %1 = bitcast %A* %a2 to i8*
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %0, i8* align 4 %1, i64 4, i1 false), !tbaa !4  ; TAG_A
+  ret void
+}
+
+%B = type { i32 (i8*, i32*, double*)** }
+
+define i32 (i8*, i32*, double*)*** @test2() {
+; CHECK-LABEL: @test2
+; CHECK-NOT: memcpy
+; CHECK: ret
+  %tmp = alloca %B, align 8
+  %tmp1 = bitcast %B* %tmp to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %tmp1, i8* align 8 undef, i64 8, i1 false), !tbaa !7  ; TAG_B
+  %tmp2 = getelementptr %B, %B* %tmp, i32 0, i32 0
+  %tmp3 = load i32 (i8*, i32*, double*)**, i32 (i8*, i32*, double*)*** %tmp2
+  ret i32 (i8*, i32*, double*)*** %tmp2
+}
+
+!0 = !{!"root"}
+!1 = !{!0, !"char"}
+!2 = !{!1, !"float"}
+!3 = !{!1, i64 4, !"A", !2, i64 0, i64 4}
+!4 = !{!3, !3, i64 0, i64 4}
+!5 = !{!1, !"pointer"}
+!6 = !{!1, i64 8, !"B", !5, i64 0, i64 8}
+!7 = !{!6, !6, i64 0, i64 8}
+
+; CHECK-DAG: [[ROOT:!.*]] = !{!"root"}
+; CHECK-DAG: [[TYPE_char:!.*]] = !{[[ROOT]], !"char"}
+; CHECK-DAG: [[TYPE_float:!.*]] = !{[[TYPE_char]], !"float"}
+; CHECK-DAG: [[TYPE_A:!.*]] = !{[[TYPE_char]], i64 4, !"A", [[TYPE_float]], i64 0, i64 4}
+; CHECK-DAG: [[TAG_A]] = !{[[TYPE_A]], [[TYPE_A]], i64 0, i64 4}
+; Note that the memcpy() call in test2() transforms into an
+; undecorated 'store undef', so TAG_B is not present in the output.