Page MenuHomePhabricator

[MemorySSA] Refactor CachingWalker.

Authored by asbirlea on Dec 20 2018, 2:29 PM.



Refactor caching walker to make creating a walker that skips the
starting access strightforward.

Diff Detail


Event Timeline

asbirlea created this revision.Dec 20 2018, 2:29 PM

Thanks for this!

976 ↗(On Diff #179158)

ClobberWalker is pretty big (nearly 2K). Can we instead make SkipSelfWalker carry around a pointer to MSSA->getWalker() (or just MSSA. I'm not picky)?

Since CachingWalker is opaque (modulo overrides from MemorySSAWalker) to everything that isn't in this file, I have no issue with adding "don't touch these unless ..." methods to its public API.

977 ↗(On Diff #179158)

please clang-format this

979 ↗(On Diff #179158)

nit: no trailing semi, please

1462 ↗(On Diff #179158)

No trailing ;, please

2275 ↗(On Diff #179158)

Please do the refactoring/dedup as a part of this patch, rather than as a follow up

asbirlea updated this revision to Diff 179985.Jan 2 2019, 5:40 PM
asbirlea marked 2 inline comments as done.

Repurpose patch to refactor caching walker.

asbirlea retitled this revision from [MemorySSA] Create walker that skips starting access. to [MemorySSA] Refactor CachingWalker..Jan 2 2019, 5:41 PM
asbirlea edited the summary of this revision. (Show Details)
asbirlea marked an inline comment as done.

This LGTM at a high level; just a handful of nits for you, and I think this should be good to submit

961 ↗(On Diff #179985)

nit: if we don't have commentary about SkipSelf elsewhere, can we please have a description of why it exists/what it's intended to do/etc.?

965 ↗(On Diff #179985)

Since this is already a nested class, is this necessary?

I don't use friend often enough to know for sure

2225 ↗(On Diff #179985)

nit: no else after return, please

2281 ↗(On Diff #179985)

Please also note whether SkipSelf was true/false in these debug printouts

2287 ↗(On Diff #179985)

nit: if member functions are one or two lines like this, please define them in the class itself (unless there's some code ordering-related reason not to)

2293 ↗(On Diff #179985)

please clang-format (I think it prefers empty lines between functions)

asbirlea updated this revision to Diff 180102.Jan 3 2019, 10:42 AM
asbirlea marked 9 inline comments as done.

Address comments.

asbirlea added inline comments.Jan 3 2019, 10:42 AM
2293 ↗(On Diff #179985)

clang-format doesn't care about this, but I added a line, seems better.


2293 ↗(On Diff #179985)

Interesting -- TIL.

This revision is now accepted and ready to land.Jan 3 2019, 11:18 AM
This revision was automatically updated to reflect the committed changes.