Issue 18740045: Improve inlining expansion with common patterns.

Issue 18740045: Improve inlining expansion with common patterns. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 6 months ago by etienneb

Modified:
10 years, 6 months ago

Reviewers:
chrisha

CC:
sawbuck-changes_googlegroups.com

Base URL:
http://sawbuck.googlecode.com/svn/trunk/

Visibility:
Public.

More Reviews

Description

Improve inlining expansion with common patterns. R=chrisha@chromium.org Committed: https://code.google.com/p/sawbuck/source/detail?r=1876

Patch Set 2 : add unitest for trampoline #

Patch Set 4 : fix chrisha comments. #

Total comments: 10

Patch Set 5 : fix chrisha comments. #

Created: 10 years, 6 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+263 lines, -22 lines)			Patch
	M	syzygy/optimize/transforms/inlining_transform.cc	View	1 2 3 4	6 chunks	+143 lines, -21 lines	0 comments	Download
	M	syzygy/optimize/transforms/inlining_transform_unittest.cc	View	1 2 3 4	7 chunks	+120 lines, -1 line	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

chrisha

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms/inlining_transform.cc File syzygy/optimize/transforms/inlining_transform.cc (right): https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms/inlining_transform.cc#newcode60 syzygy/optimize/transforms/inlining_transform.cc:60: const uint8 kEmptyBody1[] = { 0xc3 }; A comment ...

10 years, 6 months ago (2013-10-31 20:58:13 UTC) #2

etienneb

PTAnL. https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms/inlining_transform.cc File syzygy/optimize/transforms/inlining_transform.cc (right): https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms/inlining_transform.cc#newcode60 syzygy/optimize/transforms/inlining_transform.cc:60: const uint8 kEmptyBody1[] = { 0xc3 }; On ...

10 years, 6 months ago (2013-10-31 21:50:26 UTC) #3

PTAnL.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
File syzygy/optimize/transforms/inlining_transform.cc (right):

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform.cc:60: const uint8 kEmptyBody1[] =
{ 0xc3 };
On 2013/10/31 20:58:14, chrisha wrote:
> A comment about these? That they're common enough you're matching them by
> pattern rather than disassembling and analyzing?

Done.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform.cc:74: 0x55, 0x8b, 0xec, 0x8b,
0x45, 0x04, 0x5d, 0xc3 };
On 2013/10/31 20:58:14, chrisha wrote:
> We generally stick to CAPS for hex numbers.

Done.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform.cc:122: bool
MatchEmptyBody(BlockGraph::Block* callee) {
The matched function are small enough.
They aren't lot of them (and will not).

On 2013/10/31 20:58:14, chrisha wrote:
> Make this go faster by memoizing the results? ie: if you've matched to
> kEmptyBody1 once before, it's a simple lookup in a map to determine that
again?

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform.cc:132: }
Caching the result implies a hashmap proportional to N-functions.
Looking to bytes aren't inefficient as we check the size first.
And, only small function may be candidates to matching.

In fact, decomposition could be memorize. And, will probably a CL soon.

On 2013/10/31 20:58:14, chrisha wrote:
> Change the whole pattern matching paradigm? Calculate a hash of the block
> contents (at most once, caching the result), calculate the hashes of each
> pattern and put them in a map returning a type? (kEmptyBodyPattern,
> kGetProgramCounterPattern)
> 
> Should avoid looking at bytes over and over again, and unify the logic for
> handling these. Adding new patterns amounts to registering them in the hash
and
> maybe adding a new enum entry?

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
File syzygy/optimize/transforms/inlining_transform_unittest.cc (right):

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform_unittest.cc:47: kDirectTrampoline,
On 2013/10/31 20:58:14, chrisha wrote:
> Can you document what you mean by these? Maybe give small examples?

Done.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform_unittest.cc:122: CalleeKind kind,
On 2013/10/31 20:58:14, chrisha wrote:
> All output params after all input params. Move |kind| up?

Done.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform_unittest.cc:145: // Produce a
callee block. The contains of the body is determine by |kind|.
On 2013/10/31 20:58:14, chrisha wrote:
> contents* of the body are* determined*

Done.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform_unittest.cc:148:
BlockGraph::Block* target) {
On 2013/10/31 20:58:14, chrisha wrote:
> Ditto with param order.

Done.

https://codereview.appspot.com/18740045/diff/60001/syzygy/optimize/transforms...
syzygy/optimize/transforms/inlining_transform_unittest.cc:293:
TEST_F(InliningTransformTest, InlineEmptyBody1) {
On 2013/10/31 20:58:14, chrisha wrote:
> Why InlineEmptyBody*1*? What is the 1 for?

Done.

chrisha

lgtm with nits! https://codereview.appspot.com/18740045/diff/80001/syzygy/optimize/transforms/inlining_transform.cc File syzygy/optimize/transforms/inlining_transform.cc (right): https://codereview.appspot.com/18740045/diff/80001/syzygy/optimize/transforms/inlining_transform.cc#newcode64 syzygy/optimize/transforms/inlining_transform.cc:64: const uint8 kEmptyBody1[] = { 0xc3 ...

10 years, 6 months ago (2013-11-01 13:58:05 UTC) #4

etienneb

thanks, committing soon. https://codereview.appspot.com/18740045/diff/80001/syzygy/optimize/transforms/inlining_transform.cc File syzygy/optimize/transforms/inlining_transform.cc (right): https://codereview.appspot.com/18740045/diff/80001/syzygy/optimize/transforms/inlining_transform.cc#newcode64 syzygy/optimize/transforms/inlining_transform.cc:64: const uint8 kEmptyBody1[] = { 0xc3 ...

10 years, 6 months ago (2013-11-01 15:29:08 UTC) #5

etienneb

10 years, 6 months ago (2013-11-01 15:33:10 UTC) #6

Message was sent while issue was closed.

Committed patchset #5 manually as r1876 (presubmit successful).

Expand All Messages | Collapse All Messages