This is an archive of the discontinued LLVM Phabricator instance.

Add official documentation™ for MemorySSA
ClosedPublic

Authored by george.burgess.iv on Aug 15 2016, 4:43 PM.

Details

Summary

By popular demand, this patch adds documentation for MemorySSA.

This is the first time that I'm writing .rst docs, so apologies if I'm missing anything obvious. :)

Opinions on clarity/correctness are highly appreciated.

Diff Detail

Repository
rL LLVM

Event Timeline

george.burgess.iv retitled this revision from to Add official documentation™ for MemorySSA.
george.burgess.iv updated this object.
george.burgess.iv added a reviewer: dberlin.
dberlin edited edge metadata.Aug 15 2016, 5:08 PM

Thanks for doing this!
I took a first pass.
I can write up a section that describes the design in terms of SSA, etc.

docs/MemorySSA.rst
20 ↗(On Diff #68113)

It's not just clobbers, it also gives uses.

I would just write this sentence as "At a high level, one of the goals ... is to provide an SSA based form for memory that enables users to quickly find clobbers and uses of memory operations".

29 ↗(On Diff #68113)

It's worth noting: this paper is now badly out of date, and in fact, LLVM and GCC use the same form (one variable to represent the heap).

The only difference is we point uses at the nearest clobber, and gcc doesn't.

I can write up a section on the design if you want.

44 ↗(On Diff #68113)

They are not basically, they are phinodes :)

51 ↗(On Diff #68113)

This "will always hand back the same result" is .... not correct

Proof: Load in a loop :)
You mean to say "are operations which use but do not modify memory".

55 ↗(On Diff #68113)

i would go with "or otherwise clobber memory in unquantifiable ways".

61 ↗(On Diff #68113)

Use of it implies that the memory being used either is undefined or defined before the function begins.

117 ↗(On Diff #68113)

By using preceding, you are implying an order to the definitions that does not exist :)

They are not preceding definitions, they are reaching definitions.

  • `2 = MemoryDef(6) notes that store i8 0, i8* %p1 is a definition, and the definition immediately before it is 6, or the MemoryPhi after while.cond`.

This should be "and the previous reaching definition of memory is "6""

192 ↗(On Diff #68113)

This is not correct.
We do not optimize memorydefs because it would require multiple memoryphi nodes, because it generates multiple reaching definitions.

block1:
5 = memorydef(4)
store %a
6 = memorydef(5)
store %b

block2:
7 = memorydef(4)
store %a
8 = memorydef(7)
store %b

merge:
load %a
load %b

imagine %a and %b do not conflict.

In the above form, where we have not optimized, there is a single memoryphi in merge, and both loads use it.
Yay
When we optimize loads, they still point to the phi.
Yay.

If we optimize stores, watch this:

block1:
5 = memorydef(4)
store %a
6 = memorydef(liveonentry)
store %b

block2:
7 = memorydef(4)
store %a
8 = memorydef(liveonentry)
store %b

merge:
load %a
load %b

So now you've created a situation where 2 reaching definitions in each block can reach the loads, because defs create new reaching definitions, and you've made these not chained to each other. In effect, you've partitioned the heap

You cannot represent this properly without two phi nodes.

299 ↗(On Diff #68113)

and all other production implementations have given on partitioning

301 ↗(On Diff #68113)

and if you can't do it accurately, it's literally not worth doing at all.

Generating more virtual IR to talk to get "not accurate results" causes significant slowdowns for no good reason.

george.burgess.iv edited edge metadata.
george.burgess.iv marked 10 inline comments as done.

Addressed all feedback

I can write up a section that describes the design in terms of SSA, etc.

Yes please.

docs/MemorySSA.rst
29 ↗(On Diff #68113)

I can write up a section on the design if you want.

Feel free!

51 ↗(On Diff #68113)

are operations which use but do not modify memory

I had that initially, but was trying to be fancy because we model some loads as MemoryDefs, and those technically don't modify memory. Guess that wasn't the best idea :)

117 ↗(On Diff #68113)

Today I learned about reaching definitions. Neat!

I reworded a number of these bullets, and have never used "reaching definition" before, so please double-check the bullets to be sure I didn't mess the wording up. :)

192 ↗(On Diff #68113)

Neat. For the moment, I just deleted the sentence (since it served no other purpose). If you'd like me to correct it instead, I'm happy to do so.

Taking a crack at this

Your edits LGTM. Are you otherwise happy with the doc? If so, I'll commit it. :)

This revision was automatically updated to reflect the committed changes.