This is an archive of the discontinued LLVM Phabricator instance.

Differential D23846

[MemCpy] Check for alias in performMemCpyToMemSetOptzn, instead of the identity of two operands
ClosedPublic

Authored by timshen on Aug 24 2016, 11:15 AM.

Details

Reviewers
chandlerc
ab
echristo
jlebar
iteratee
Commits
rG3ad8b43cc279: [MemCpy] Add comments for r279769
rGa3dbead2d66a: [MemCpy] Check for alias in performMemCpyToMemSetOptzn, instead of the identity…
rL279778: [MemCpy] Add comments for r279769
rL279769: [MemCpy] Check for alias in performMemCpyToMemSetOptzn, instead of the…
Summary

This fixes pr29105. The issue is that lifetime marks creates new
aliasing pointers to the original ones, but AA
was not used in performMemCpyToMemSetOptzn.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen updated this revision to Diff 69145.Aug 24 2016, 11:15 AM
timshen retitled this revision from to [MemCpy] Check for alias in performMemCpyToMemSetOptzn, instead of the identity of two operands.
timshen updated this object.
timshen added a subscriber: llvm-commits.
timshen added a reviewer: ab.Aug 24 2016, 11:21 AM
timshen updated this object.Aug 25 2016, 1:02 AM
timshen added reviewers: chandlerc, echristo, iteratee, jlebar.
iteratee accepted this revision.Aug 25 2016, 11:29 AM
iteratee edited edge metadata.

MustAlias is true only for exact aliases correct?

If so, this can land.

This revision is now accepted and ready to land.Aug 25 2016, 11:29 AM
hfinkel added a subscriber: hfinkel.Aug 25 2016, 11:58 AM
In D23846#525459, @iteratee wrote:

MustAlias is true only for exact aliases correct?

Correct.

If so, this can land.

timshen added a comment.Aug 25 2016, 12:16 PM
In D23846#525459, @iteratee wrote:

MustAlias is true only for exact aliases correct?

Yes, the comment in AliasAnalysis.h says so:

/// The two locations precisely alias each other.
MustAlias,
Closed by commit rL279769: [MemCpy] Check for alias in performMemCpyToMemSetOptzn, instead of the… (authored by timshen). · Explain WhyAug 25 2016, 12:35 PM
This revision was automatically updated to reflect the committed changes.
echristo added inline comments.Aug 25 2016, 1:52 PM
llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1111

Can probably fold this into the conditional since this is the only place it's used.

1114

Document why you care about MustAlias here.

llvm/trunk/test/Transforms/MemCpyOpt/pr29105.ll
4

Comment on what you're testing.

timshen marked 2 inline comments as done.Aug 25 2016, 2:16 PM
timshen added inline comments.
llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1111

If you are talking about:

if (LookupAliasAnalysis().isMustAlias(...))

, then for consistency with other call sites, I like to keep it as is (I searched for "LookupAliasAnalysis()." and found nothing).

If you are talking about:

if (auto &AA = LoopupAliasAnalysis(); AA.isMustAlias(...))

It's a C++17 feature and we are not quite there yet! :)

echristo added inline comments.Aug 25 2016, 2:25 PM
llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1111

Feel free to fix up the other single uses at the same time? :)

That said, I don't feel that strongly here either way.

Revision Contents

PathSize
llvm/
trunk/
lib/
Transforms/
Scalar/
4 lines
test/
Transforms/
MemCpyOpt/
38 lines

Diff 69283

llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp

Show First 20 Lines • Show All 1,102 Lines • ▼ Show 20 Lines
/// memset(dst1, c, dst1_size); /// memset(dst1, c, dst1_size);
/// memset(dst2, c, dst2_size); /// memset(dst2, c, dst2_size);
/// \endcode /// \endcode
/// When dst2_size <= dst1_size. /// When dst2_size <= dst1_size.
/// ///
/// The \p MemCpy must have a Constant length. /// The \p MemCpy must have a Constant length.
bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy, bool MemCpyOptPass::performMemCpyToMemSetOptzn(MemCpyInst *MemCpy,
MemSetInst *MemSet) { MemSetInst *MemSet) {
AliasAnalysis &AA = LookupAliasAnalysis();
echristoUnsubmitted
Not Done
ReplyInline Actions

Can probably fold this into the conditional since this is the only place it's used.

echristo: Can probably fold this into the conditional since this is the only place it's used.
timshenAuthorUnsubmitted
Not Done
ReplyInline Actions

If you are talking about:

if (LookupAliasAnalysis().isMustAlias(...))

, then for consistency with other call sites, I like to keep it as is (I searched for "LookupAliasAnalysis()." and found nothing).

If you are talking about:

if (auto &AA = LoopupAliasAnalysis(); AA.isMustAlias(...))

It's a C++17 feature and we are not quite there yet! :)

timshen: If you are talking about: if (LookupAliasAnalysis().isMustAlias(...)) , then for consistency…
echristoUnsubmitted
Not Done
ReplyInline Actions

Feel free to fix up the other single uses at the same time? :)

That said, I don't feel that strongly here either way.

echristo: Feel free to fix up the other single uses at the same time? :) That said, I don't feel that…
// This only makes sense on memcpy(..., memset(...), ...). // This only makes sense on memcpy(..., memset(...), ...).
if (MemSet->getRawDest() != MemCpy->getRawSource()) if (!AA.isMustAlias(MemSet->getRawDest(), MemCpy->getRawSource()))
echristoUnsubmitted
Done
ReplyInline Actions

Document why you care about MustAlias here.

echristo: Document why you care about MustAlias here.
return false; return false;
ConstantInt *CopySize = cast<ConstantInt>(MemCpy->getLength()); ConstantInt *CopySize = cast<ConstantInt>(MemCpy->getLength());
ConstantInt *MemSetSize = dyn_cast<ConstantInt>(MemSet->getLength()); ConstantInt *MemSetSize = dyn_cast<ConstantInt>(MemSet->getLength());
// Make sure the memcpy doesn't read any more than what the memset wrote. // Make sure the memcpy doesn't read any more than what the memset wrote.
// Don't worry about sizes larger than i64. // Don't worry about sizes larger than i64.
if (!MemSetSize || CopySize->getZExtValue() > MemSetSize->getZExtValue()) if (!MemSetSize || CopySize->getZExtValue() > MemSetSize->getZExtValue())
return false; return false;
▲ Show 20 Lines • Show All 321 Lines • Show Last 20 Lines

llvm/trunk/test/Transforms/MemCpyOpt/pr29105.ll

; RUN: opt -memcpyopt -instcombine -S %s | FileCheck %s
%Foo = type { [2048 x i64] }
; CHECK-LABEL: @baz(
echristoUnsubmitted
Done
ReplyInline Actions

Comment on what you're testing.

echristo: Comment on what you're testing.
; CHECK-NOT: call void @llvm.memcpy
define void @baz() unnamed_addr #0 {
entry-block:
%x.sroa.0 = alloca [2048 x i64], align 8
%tmp0 = alloca [2048 x i64], align 8
%0 = bitcast [2048 x i64]* %tmp0 to i8*
%tmp2 = alloca %Foo, align 8
%x.sroa.0.0..sroa_cast6 = bitcast [2048 x i64]* %x.sroa.0 to i8*
call void @llvm.lifetime.start(i64 16384, i8* %x.sroa.0.0..sroa_cast6)
call void @llvm.lifetime.start(i64 16384, i8* %0)
call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 16384, i32 8, i1 false)
call void @llvm.memcpy.p0i8.p0i8.i64(i8* %x.sroa.0.0..sroa_cast6, i8* %0, i64 16384, i32 8, i1 false)
call void @llvm.lifetime.end(i64 16384, i8* %0)
%1 = bitcast %Foo* %tmp2 to i8*
call void @llvm.lifetime.start(i64 16384, i8* %1)
call void @llvm.memcpy.p0i8.p0i8.i64(i8* %1, i8* %x.sroa.0.0..sroa_cast6, i64 16384, i32 8, i1 false)
call void @bar(%Foo* noalias nocapture nonnull dereferenceable(16384) %tmp2)
call void @llvm.lifetime.end(i64 16384, i8* %1)
call void @llvm.lifetime.end(i64 16384, i8* %x.sroa.0.0..sroa_cast6)
ret void
}
declare void @llvm.lifetime.start(i64, i8* nocapture) #1
declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1) #1
declare void @llvm.lifetime.end(i64, i8* nocapture) #1
declare void @bar(%Foo* noalias nocapture readonly dereferenceable(16384)) unnamed_addr #0
declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i32, i1) #1
attributes #0 = { uwtable }
attributes #1 = { argmemonly nounwind }