This is an archive of the discontinued LLVM Phabricator instance.

[StackProtector] Speed up RequiresStackProtector
ClosedPublic

Authored by nadav on Jul 26 2020, 11:30 PM.

Download Raw Diff

Details

Reviewers

bricci
craig.topper

Commits

rGdf880b77302d: [StackProtector] Speed up RequiresStackProtector

Summary

Speed up the method RequiresStackProtector by checking the intrinsic
value of the call. The original code calls getName() that returns an
allocating std::string on each check. This change removes about 96072 std::string
instances when compiling sqlite3.c; The function was discovered with a
Facebook-internal performance tool.

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed

	Time	Test
	140 ms	linux > Polly.ScopInfo::Unknown Unit Message ("")

Event Timeline

nadav created this revision.Jul 26 2020, 11:30 PM

Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2020, 11:30 PM

Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript

nadav added a reviewer: bricci.Jul 26 2020, 11:31 PM

craig.topper added a subscriber: craig.topper.Jul 26 2020, 11:41 PM

craig.topper added inline comments.

llvm/lib/CodeGen/StackProtector.cpp
257	Can this just be CI->getIntrinsicID() == Intrinsic::stackprotector?

Harbormaster failed remote builds in B65765: Diff 280791!Jul 27 2020, 12:01 AM

@craig.topper Good catch. Thank you. I've updated the diff.

nadav edited the summary of this revision. (Show Details)Jul 27 2020, 8:17 AM

nadav updated this revision to Diff 280940.Jul 27 2020, 9:09 AM

Harbormaster failed remote builds in B65836: Diff 280924!Jul 27 2020, 9:28 AM

LGTM

This revision is now accepted and ready to land.Jul 27 2020, 9:36 AM

Closed by commit rGdf880b77302d: [StackProtector] Speed up RequiresStackProtector (authored by nadav). · Explain WhyJul 27 2020, 10:14 AM

This revision was automatically updated to reflect the committed changes.

Harbormaster failed remote builds in B65844: Diff 280940!Jul 27 2020, 10:28 AM

Revision Contents

Path

Size

llvm/

lib/

CodeGen/

StackProtector.cpp

7 lines

Diff 280924

llvm/lib/CodeGen/StackProtector.cpp

Context not available.
	static const CallInst *findStackProtectorIntrinsic(Function &F) {	static const CallInst *findStackProtectorIntrinsic(Function &F) {
	for (const BasicBlock &BB : F)	for (const BasicBlock &BB : F)
	for (const Instruction &I : BB)	for (const Instruction &I : BB)
	if (const CallInst *CI = dyn_cast<CallInst>(&I))	if (auto *II = dyn_cast<IntrinsicInst>(&I))
		Lint: Pre-merge checks Inline Actions clang-tidy: warning: 'auto II' can be declared as 'const auto II' [llvm-qualified-auto] not useful Lint: Pre-merge checks: clang-tidy: warning: 'auto II' can be declared as 'const auto II' [llvm-qualified-auto]…
	if (CI->getCalledFunction() ==	if (II->getIntrinsicID() == Intrinsic::stackprotector)
	Intrinsic::getDeclaration(F.getParent(), Intrinsic::stackprotector))	return II;
	return CI;
	return nullptr;	return nullptr;
		craig.topperUnsubmitted Not Done Reply Inline Actions Can this just be CI->getIntrinsicID() == Intrinsic::stackprotector? craig.topper: Can this just be CI->getIntrinsicID() == Intrinsic::stackprotector?
	}	}

Context not available.