This is an archive of the discontinued LLVM Phabricator instance.

Differential D36844

[PGO] Fixed assertion due to mismatched memcpy size type.
ClosedPublic

Authored by apazos on Aug 17 2017, 12:48 PM.

Details

Reviewers
davidxl
xur
Commits
rG6210f27dfc37: [PGO] Fixed assertion due to mismatched memcpy size type.
rL311188: [PGO] Fixed assertion due to mismatched memcpy size type.
Summary

Memcpy intrinsics have size argument of any integer type, like i32 or i64.
Fixed size type along with its value when cloning the intrinsic.

Diff Detail

Event Timeline

apazos created this revision.Aug 17 2017, 12:48 PM
apazos added reviewers: davidxl, xur.Aug 17 2017, 12:52 PM
apazos added a subscriber: llvm-commits.
apazos added a comment.Aug 17 2017, 4:24 PM

Added llvm-commits.

davidxl accepted this revision.Aug 17 2017, 4:42 PM

lgtm

This revision is now accepted and ready to land.Aug 17 2017, 4:42 PM
xur edited edge metadata.Aug 18 2017, 9:09 AM

Looks good to me. Thanks for fixing.

mcrosier added a subscriber: mcrosier.Aug 18 2017, 9:30 AM
mcrosier added inline comments.
lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp
368

I believe this can be just a cast, rather than a dyn_cast (we know the cloned instruction is a MemIntrinsic).

370

You might consider adding a message within the assert:
assert(SizeType && "Expected an integer size type.");

apazos updated this revision to Diff 111706.Aug 18 2017, 11:09 AM
apazos marked 2 inline comments as done.
apazos edited the summary of this revision. (Show Details)

Added assert message.

apazos marked an inline comment as not done.Aug 18 2017, 11:09 AM
Closed by commit rL311188: [PGO] Fixed assertion due to mismatched memcpy size type. (authored by apazos). · Explain WhyAug 18 2017, 12:18 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Transforms/
Instrumentation/
7 lines
test/
Transforms/
PGOProfile/
27 lines

Diff 111545

lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp

Show First 20 Lines • Show All 355 Lines • ▼ Show 20 Linesbool MemOPSizeOpt::perform(MemIntrinsic *MI) {
if (SavedRemainCount > 0 || Version != NumVals) if (SavedRemainCount > 0 || Version != NumVals)
// Otherwise we need update with the un-promoted records back. // Otherwise we need update with the un-promoted records back.
annotateValueSite(*Func.getParent(), *MI, VDs.slice(Version), annotateValueSite(*Func.getParent(), *MI, VDs.slice(Version),
SavedRemainCount, IPVK_MemOPSize, NumVals); SavedRemainCount, IPVK_MemOPSize, NumVals);
DEBUG(dbgs() << "\n\n== Basic Block After==\n"); DEBUG(dbgs() << "\n\n== Basic Block After==\n");
for (uint64_t SizeId : SizeIds) { for (uint64_t SizeId : SizeIds) {
ConstantInt *CaseSizeId = ConstantInt::get(Type::getInt64Ty(Ctx), SizeId);
BasicBlock *CaseBB = BasicBlock::Create( BasicBlock *CaseBB = BasicBlock::Create(
Ctx, Twine("MemOP.Case.") + Twine(SizeId), &Func, DefaultBB); Ctx, Twine("MemOP.Case.") + Twine(SizeId), &Func, DefaultBB);
Instruction *NewInst = MI->clone(); Instruction *NewInst = MI->clone();
// Fix the argument. // Fix the argument.
dyn_cast<MemIntrinsic>(NewInst)->setLength(CaseSizeId); MemIntrinsic * MemI = dyn_cast<MemIntrinsic>(NewInst);
mcrosierUnsubmitted
Not Done
ReplyInline Actions

I believe this can be just a cast, rather than a dyn_cast (we know the cloned instruction is a MemIntrinsic).

mcrosier: I believe this can be just a cast, rather than a dyn_cast (we know the cloned instruction is a…
IntegerType *SizeType = dyn_cast<IntegerType>(MemI->getLength()->getType());
assert(SizeType);
mcrosierUnsubmitted
Done
ReplyInline Actions

You might consider adding a message within the assert:
assert(SizeType && "Expected an integer size type.");

mcrosier: You might consider adding a message within the assert: assert(SizeType && "Expected an integer…
ConstantInt *CaseSizeId = ConstantInt::get(SizeType, SizeId);
MemI->setLength(CaseSizeId);
CaseBB->getInstList().push_back(NewInst); CaseBB->getInstList().push_back(NewInst);
IRBuilder<> IRBCase(CaseBB); IRBuilder<> IRBCase(CaseBB);
IRBCase.CreateBr(MergeBB); IRBCase.CreateBr(MergeBB);
SI->addCase(CaseSizeId, CaseBB); SI->addCase(CaseSizeId, CaseBB);
DEBUG(dbgs() << *CaseBB << "\n"); DEBUG(dbgs() << *CaseBB << "\n");
} }
setProfMetadata(Func.getParent(), SI, CaseCounts, MaxCount); setProfMetadata(Func.getParent(), SI, CaseCounts, MaxCount);
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines

test/Transforms/PGOProfile/memop_clone.ll

  • This file was added.
; RUN: opt < %s -pgo-memop-opt -S | FileCheck %s
define i32 @test(i8* %a, i8* %b) !prof !1 {
; CHECK_LABEL: test
; CHECK: MemOP.Case.3:
; CHECK: tail call void @llvm.memcpy.p0i8.p0i8.i32(i8* undef, i8* %a, i32 3, i32 1, i1 false)
; CHECK: MemOP.Case.2:
; CHECK: tail call void @llvm.memcpy.p0i8.p0i8.i32(i8* undef, i8* %a, i32 2, i32 1, i1 false)
; CHECK: MemOP.Default:
; CHECK: tail call void @llvm.memcpy.p0i8.p0i8.i32(i8* undef, i8* %a, i32 undef, i32 1, i1 false)
; CHECK: MemOP.Case.33:
; CHECK: tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* undef, i8* %b, i64 3, i32 1, i1 false)
; CHECK MemOP.Case.24:
; CHECK: tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* undef, i8* %b, i64 2, i32 1, i1 false)
; CHECK: MemOP.Default2:
; CHECK: tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* undef, i8* %b, i64 undef, i32 1, i1 false)
tail call void @llvm.memcpy.p0i8.p0i8.i32(i8* undef, i8* %a, i32 undef, i32 1, i1 false), !prof !2
tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* undef, i8* %b, i64 undef, i32 1, i1 false), !prof !2
unreachable
}
declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture writeonly, i8* nocapture readonly, i32, i32, i1)
declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1)
!1 = !{!"function_entry_count", i64 5170}
!2 = !{!"VP", i32 1, i64 2585, i64 3, i64 1802, i64 2, i64 783}