This is an archive of the discontinued LLVM Phabricator instance.

Differential D89321

[MemLoc] Support memchr/memccpy in MemoryLocation::getForArgument
ClosedPublic

Authored by xbolva00 on Oct 13 2020, 8:29 AM.

Details

Reviewers
fhahn
Commits
rG28691cdd714f: [MemLoc] Support memchr/memccpy in MemoryLocation::getForArgument

Diff Detail

Unit TestsFailed

TimeTest
3,940 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.AddFiles
3,720 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.DeleteFile
3,730 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.ModifyFile

Event Timeline

xbolva00 created this revision.Oct 13 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2020, 8:29 AM
Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript
xbolva00 requested review of this revision.Oct 13 2020, 8:29 AM
Harbormaster completed remote builds in B74941: Diff 297872.Oct 13 2020, 9:17 AM
fhahn accepted this revision.Oct 16 2020, 1:44 AM

LGTM, thanks!

llvm/test/Transforms/DeadStoreElimination/MSSA/libcalls.ll
277

nit: noalias should not be needed here and the other tests, as %stack.ptr is to an alloca and LLVM should be able to determine noalias without the extra help.

This revision is now accepted and ready to land.Oct 16 2020, 1:44 AM
This revision was landed with ongoing or failed builds.Oct 16 2020, 2:38 AM
Closed by commit rG28691cdd714f: [MemLoc] Support memchr/memccpy in MemoryLocation::getForArgument (authored by xbolva00). · Explain Why
This revision was automatically updated to reflect the committed changes.
xbolva00 added a commit: rG28691cdd714f: [MemLoc] Support memchr/memccpy in MemoryLocation::getForArgument.
xbolva00 marked an inline comment as done.Oct 16 2020, 2:39 AM
fhahn added inline comments.Oct 21 2020, 2:35 AM
llvm/lib/Analysis/MemoryLocation.cpp
262

Thinking about this again, I am not sure if this is a good idea to do for the destination argument, because it gives the impression that exactly LenCI bytes are written, when this is not the case.

I think for reads the larger size is the conservative option and fine, but for writing it could mis-lead clients into thinking the call writes more bytes than it actually does, leading to potentially invalid optimizations.

xbolva00 added inline comments.Oct 21 2020, 2:52 AM
llvm/lib/Analysis/MemoryLocation.cpp
262

Makes sense, I will prepare fix to skip doing this for dst.

Revision Contents

PathSize
llvm/
lib/
Analysis/
15 lines
test/
Analysis/
BasicAA/
40 lines
Transforms/
DeadStoreElimination/
MSSA/
112 lines

Diff 297872

llvm/lib/Analysis/MemoryLocation.cpp

Show First 20 Lines • Show All 241 Lines • ▼ Show 20 Lines if (TLI && TLI->getLibFunc(*Call, F) && TLI->has(F)) {
case LibFunc_memcmp: case LibFunc_memcmp:
assert((ArgIdx == 0 || ArgIdx == 1) && assert((ArgIdx == 0 || ArgIdx == 1) &&
"Invalid argument index for memcmp/bcmp"); "Invalid argument index for memcmp/bcmp");
if (const ConstantInt *LenCI = if (const ConstantInt *LenCI =
dyn_cast<ConstantInt>(Call->getArgOperand(2))) dyn_cast<ConstantInt>(Call->getArgOperand(2)))
return MemoryLocation(Arg, LocationSize::precise(LenCI->getZExtValue()), return MemoryLocation(Arg, LocationSize::precise(LenCI->getZExtValue()),
AATags); AATags);
break; break;
case LibFunc_memchr:
assert((ArgIdx == 0) && "Invalid argument index for memchr");
if (const ConstantInt *LenCI =
dyn_cast<ConstantInt>(Call->getArgOperand(2)))
return MemoryLocation(Arg, LocationSize::precise(LenCI->getZExtValue()),
AATags);
break;
case LibFunc_memccpy:
assert((ArgIdx == 0 || ArgIdx == 1) &&
"Invalid argument index for memccpy");
if (const ConstantInt *LenCI =
dyn_cast<ConstantInt>(Call->getArgOperand(3)))
return MemoryLocation(Arg, LocationSize::precise(LenCI->getZExtValue()),
fhahnUnsubmitted
Not Done
ReplyInline Actions

Thinking about this again, I am not sure if this is a good idea to do for the destination argument, because it gives the impression that exactly LenCI bytes are written, when this is not the case.

I think for reads the larger size is the conservative option and fine, but for writing it could mis-lead clients into thinking the call writes more bytes than it actually does, leading to potentially invalid optimizations.

fhahn: Thinking about this again, I am not sure if this is a good idea to do for the destination…
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

Makes sense, I will prepare fix to skip doing this for dst.

xbolva00: Makes sense, I will prepare fix to skip doing this for dst.
AATags);
break;
default: default:
break; break;
}; };
} }
// FIXME: Handle memset_pattern4 and memset_pattern8 also. // FIXME: Handle memset_pattern4 and memset_pattern8 also.
return MemoryLocation(Call->getArgOperand(ArgIdx), LocationSize::unknown(), return MemoryLocation(Call->getArgOperand(ArgIdx), LocationSize::unknown(),
AATags); AATags);
} }

llvm/test/Analysis/BasicAA/libfuncs.ll

Show First 20 Lines • Show All 80 Lines • ▼ Show 20 Linesentry:
%a.gep.5 = getelementptr i8, i8* %a, i32 5 %a.gep.5 = getelementptr i8, i8* %a, i32 5
store i8 1, i8* %a.gep.5 store i8 1, i8* %a.gep.5
%b.gep.1 = getelementptr i8, i8* %b, i32 1 %b.gep.1 = getelementptr i8, i8* %b, i32 1
store i8 2, i8* %b.gep.1 store i8 2, i8* %b.gep.1
%b.gep.5 = getelementptr i8, i8* %b, i32 5 %b.gep.5 = getelementptr i8, i8* %b, i32 5
store i8 3, i8* %b.gep.5 store i8 3, i8* %b.gep.5
ret i32 %res ret i32 %res
} }
declare i8* @memchr(i8*, i32, i64)
; CHECK-LABEL: Function: test_memchr_const_size
; CHECK: Just Ref: Ptr: i8* %res <-> %res = call i8* @memchr(i8* %a, i32 42, i64 4)
; CHECK-NEXT: Just Ref: Ptr: i8* %a.gep.1 <-> %res = call i8* @memchr(i8* %a, i32 42, i64 4)
; CHECK-NEXT: NoModRef: Ptr: i8* %a.gep.5 <-> %res = call i8* @memchr(i8* %a, i32 42, i64 4)
define i8* @test_memchr_const_size(i8* noalias %a) {
entry:
%res = call i8* @memchr(i8* %a, i32 42, i64 4)
%a.gep.1 = getelementptr i8, i8* %a, i32 1
store i8 0, i8* %a.gep.1
%a.gep.5 = getelementptr i8, i8* %a, i32 5
store i8 1, i8* %a.gep.5
ret i8* %res
}
declare i8* @memccpy(i8*, i8*, i32, i64)
; CHECK-LABEL: Function: test_memccpy_const_size
; CHECK: Both ModRef: Ptr: i8* %a <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
; CHECK-NEXT: Just Ref: Ptr: i8* %b <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
; CHECK-NEXT: Both ModRef: Ptr: i8* %res <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
; CHECK-NEXT: Both ModRef: Ptr: i8* %a.gep.1 <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
; CHECK-NEXT: NoModRef: Ptr: i8* %a.gep.5 <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
; CHECK-NEXT: Just Ref: Ptr: i8* %b.gep.1 <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
; CHECK-NEXT: NoModRef: Ptr: i8* %b.gep.5 <-> %res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
define i8* @test_memccpy_const_size(i8* noalias %a, i8* noalias %b) {
entry:
%res = call i8* @memccpy(i8* %a, i8* %b, i32 42, i64 4)
%a.gep.1 = getelementptr i8, i8* %a, i32 1
store i8 0, i8* %a.gep.1
%a.gep.5 = getelementptr i8, i8* %a, i32 5
store i8 1, i8* %a.gep.5
%b.gep.1 = getelementptr i8, i8* %b, i32 1
store i8 2, i8* %b.gep.1
%b.gep.5 = getelementptr i8, i8* %b, i32 5
store i8 3, i8* %b.gep.5
ret i8* %res
}
No newline at end of file

llvm/test/Transforms/DeadStoreElimination/MSSA/libcalls.ll

Show First 20 Lines • Show All 184 Lines • ▼ Show 20 Linesentry:
%gep.2 = getelementptr i8, i8* %stack.ptr, i64 2 %gep.2 = getelementptr i8, i8* %stack.ptr, i64 2
store i8 51, i8* %gep.2, align 1 store i8 51, i8* %gep.2, align 1
%gep.3 = getelementptr i8, i8* %stack.ptr, i64 3 %gep.3 = getelementptr i8, i8* %stack.ptr, i64 3
store i8 52, i8* %gep.3, align 1 store i8 52, i8* %gep.3, align 1
%call = call i32 @bcmp(i8* nonnull %foo, i8* nonnull %stack.ptr, i64 %n) %call = call i32 @bcmp(i8* nonnull %foo, i8* nonnull %stack.ptr, i64 %n)
%res = icmp eq i32 %call, 0 %res = icmp eq i32 %call, 0
ret i1 %res ret i1 %res
} }
declare i8* @memchr(i8*, i32, i64)
define i8* @test_memchr_const_size() {
; CHECK-LABEL: @test_memchr_const_size(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[STACK:%.*]] = alloca [10 x i8], align 1
; CHECK-NEXT: [[STACK_PTR:%.*]] = bitcast [10 x i8]* [[STACK]] to i8*
; CHECK-NEXT: store i8 49, i8* [[STACK_PTR]], align 1
; CHECK-NEXT: [[GEP_1:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 1
; CHECK-NEXT: store i8 50, i8* [[GEP_1]], align 1
; CHECK-NEXT: [[CALL:%.*]] = call i8* @memchr(i8* [[STACK_PTR]], i32 42, i64 2)
; CHECK-NEXT: ret i8* [[CALL]]
;
entry:
%stack = alloca [10 x i8]
%stack.ptr = bitcast [10 x i8]* %stack to i8*
store i8 49, i8* %stack.ptr, align 1
%gep.1 = getelementptr i8, i8* %stack.ptr, i64 1
store i8 50, i8* %gep.1, align 1
%gep.2 = getelementptr i8, i8* %stack.ptr, i64 2
store i8 51, i8* %gep.2, align 1
%gep.3 = getelementptr i8, i8* %stack.ptr, i64 3
store i8 52, i8* %gep.3, align 1
%call = call i8* @memchr(i8* %stack.ptr, i32 42, i64 2)
ret i8* %call
}
define i8* @test_memchr_variable_size(i64 %n) {
; CHECK-LABEL: @test_memchr_variable_size(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[STACK:%.*]] = alloca [10 x i8], align 1
; CHECK-NEXT: [[STACK_PTR:%.*]] = bitcast [10 x i8]* [[STACK]] to i8*
; CHECK-NEXT: store i8 49, i8* [[STACK_PTR]], align 1
; CHECK-NEXT: [[GEP_1:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 1
; CHECK-NEXT: store i8 50, i8* [[GEP_1]], align 1
; CHECK-NEXT: [[GEP_2:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 2
; CHECK-NEXT: store i8 51, i8* [[GEP_2]], align 1
; CHECK-NEXT: [[GEP:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 4
; CHECK-NEXT: store i8 52, i8* [[GEP]], align 1
; CHECK-NEXT: [[CALL:%.*]] = call i8* @memchr(i8* [[STACK_PTR]], i32 42, i64 [[N:%.*]])
; CHECK-NEXT: ret i8* [[CALL]]
;
entry:
%stack = alloca [10 x i8]
%stack.ptr = bitcast [10 x i8]* %stack to i8*
store i8 49, i8* %stack.ptr, align 1
%gep.1 = getelementptr i8, i8* %stack.ptr, i64 1
store i8 50, i8* %gep.1, align 1
%gep.2 = getelementptr i8, i8* %stack.ptr, i64 2
store i8 51, i8* %gep.2, align 1
%gep = getelementptr i8, i8* %stack.ptr, i64 4
store i8 52, i8* %gep, align 1
%call = call i8* @memchr(i8* %stack.ptr, i32 42, i64 %n)
ret i8* %call
}
declare i8* @memccpy(i8*, i8*, i32, i64)
define i8* @test_memccpy_const_size(i8* noalias %foo) {
; CHECK-LABEL: @test_memccpy_const_size(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[STACK:%.*]] = alloca [10 x i8], align 1
; CHECK-NEXT: [[STACK_PTR:%.*]] = bitcast [10 x i8]* [[STACK]] to i8*
; CHECK-NEXT: store i8 49, i8* [[STACK_PTR]], align 1
; CHECK-NEXT: [[GEP_1:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 1
; CHECK-NEXT: store i8 50, i8* [[GEP_1]], align 1
; CHECK-NEXT: [[RES:%.*]] = call i8* @memccpy(i8* [[FOO:%.*]], i8* [[STACK_PTR]], i32 42, i64 2)
; CHECK-NEXT: ret i8* [[RES]]
;
entry:
%stack = alloca [10 x i8]
%stack.ptr = bitcast [10 x i8]* %stack to i8*
store i8 49, i8* %stack.ptr, align 1
%gep.1 = getelementptr i8, i8* %stack.ptr, i64 1
store i8 50, i8* %gep.1, align 1
%gep.2 = getelementptr i8, i8* %stack.ptr, i64 2
store i8 51, i8* %gep.2, align 1
%gep.3 = getelementptr i8, i8* %stack.ptr, i64 3
store i8 52, i8* %gep.3, align 1
%res = call i8* @memccpy(i8* %foo, i8* %stack.ptr, i32 42, i64 2)
ret i8* %res
}
define i8* @test_memccpy_variable_size(i8* noalias %foo, i64 %n) {
fhahnUnsubmitted
Done
ReplyInline Actions

nit: noalias should not be needed here and the other tests, as %stack.ptr is to an alloca and LLVM should be able to determine noalias without the extra help.

fhahn: nit: noalias should not be needed here and the other tests, as %stack.ptr is to an alloca and…
; CHECK-LABEL: @test_memccpy_variable_size(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[STACK:%.*]] = alloca [10 x i8], align 1
; CHECK-NEXT: [[STACK_PTR:%.*]] = bitcast [10 x i8]* [[STACK]] to i8*
; CHECK-NEXT: store i8 49, i8* [[STACK_PTR]], align 1
; CHECK-NEXT: [[GEP_1:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 1
; CHECK-NEXT: store i8 50, i8* [[GEP_1]], align 1
; CHECK-NEXT: [[GEP_2:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 2
; CHECK-NEXT: store i8 51, i8* [[GEP_2]], align 1
; CHECK-NEXT: [[GEP_3:%.*]] = getelementptr i8, i8* [[STACK_PTR]], i64 3
; CHECK-NEXT: store i8 52, i8* [[GEP_3]], align 1
; CHECK-NEXT: [[RES:%.*]] = call i8* @memccpy(i8* [[FOO:%.*]], i8* [[STACK_PTR]], i32 42, i64 [[N:%.*]])
; CHECK-NEXT: ret i8* [[RES]]
;
entry:
%stack = alloca [10 x i8]
%stack.ptr = bitcast [10 x i8]* %stack to i8*
store i8 49, i8* %stack.ptr, align 1
%gep.1 = getelementptr i8, i8* %stack.ptr, i64 1
store i8 50, i8* %gep.1, align 1
%gep.2 = getelementptr i8, i8* %stack.ptr, i64 2
store i8 51, i8* %gep.2, align 1
%gep.3 = getelementptr i8, i8* %stack.ptr, i64 3
store i8 52, i8* %gep.3, align 1
%res = call i8* @memccpy(i8* %foo, i8* %stack.ptr, i32 42, i64 %n)
ret i8* %res
}