This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Add APIs to move memory accesses between blocks, following CFG changes.
ClosedPublic

Authored by asbirlea on Jul 3 2018, 2:07 PM.

Details

Summary

The move APIs added in this patch will be used to update MemorySSA when CFG changes merge or split blocks, by moving memory accesses accordingly in MemorySSA's internal data structures.
[Split from D45299 for easier review]

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Jul 3 2018, 2:07 PM

Thanks for the patch!

include/llvm/Analysis/MemorySSAUpdater.h
107 ↗(On Diff #153967)

s/instructiones/instructions/

123 ↗(On Diff #153967)

Were any instructions moved as part of the split, or did we just insert New between Old and all of its predecessors? In the latter case, I'd nitpick this to something more like "BasicBlock Old had New, an empty BasicBlock, added directly before it, making Old's only predecessor New. Move Old's Phi, if present, to New."

203 ↗(On Diff #153967)

I'm assuming we have plans to make Where != End in future patches? (If so, that's OK; I'm just checking, since it looks like we're only passing in End right now)

lib/Analysis/MemorySSA.cpp
1483 ↗(On Diff #153967)

Would it also be good to assert that BB doesn't currently have a Phi?

(We could get that sort of implicitly if we used insert(...).second instead of operator[] here)

lib/Analysis/MemorySSAUpdater.cpp
438 ↗(On Diff #153967)

Prefer early exits if the code inside the if is long like this, please. (And below, at if (FirstInNew))

441 ↗(On Diff #153967)

for (Instruction &I : make_range(Start->getIterator(), To->end()) {?

448 ↗(On Diff #153967)

s/dyn_cast_or_null/cast/, since we're dereferencing it on the next line.

452 ↗(On Diff #153967)

cast, please.

456 ↗(On Diff #153967)

Is it possible to fold some of the above code into this loop without losing much in the way of clarity? It looks really similar to this loop body to me.

462 ↗(On Diff #153967)

Same nit about cast.

476 ↗(On Diff #153967)

Would for (BasicBlock *Succ : successors(To)) work? (and below)

496 ↗(On Diff #153967)

Micro style nit:

if (MemoryPhi *Phi = ...)
  MSSA->moveTo(...)
asbirlea marked 12 inline comments as done.Jul 10 2018, 11:19 AM
asbirlea added inline comments.
include/llvm/Analysis/MemorySSAUpdater.h
203 ↗(On Diff #153967)

I don't currently have a use case, so removing. I will add it again if a use case comes up.

lib/Analysis/MemorySSAUpdater.cpp
456 ↗(On Diff #153967)

I removed the insertion place so that simplified things. PTAL.

asbirlea updated this revision to Diff 154850.Jul 10 2018, 11:19 AM
asbirlea marked 2 inline comments as done.

Address comments.

LGTM with a few comments; thanks again!

lib/Analysis/MemorySSA.cpp
1483 ↗(On Diff #154850)

assert(X) compiles to nothing in non-asserting builds, so we probably want something more like:

bool Inserted = ValueToMemoryAccess.insert({BB, What}).second;
(void)Inserted;
assert(Inserted && "Cannot ...");

(the (void)Inserted; suppresses unused variable warnings, and forgetting to put it has tripped me up more times than I can count.)

lib/Analysis/MemorySSAUpdater.cpp
455 ↗(On Diff #154850)

Please add a quick comment noting that moveTo might delete Accs, so we need to re-retrieve it here

This revision is now accepted and ready to land.Jul 10 2018, 11:58 AM
asbirlea updated this revision to Diff 154868.Jul 10 2018, 1:21 PM
asbirlea marked 2 inline comments as done.

Address comments and remove movePhi API. There is a use case needing something move generic that I will send in a separate patch.

Thank you for the review!

asbirlea marked 2 inline comments as done.Jul 10 2018, 1:21 PM
This revision was automatically updated to reflect the committed changes.