[PATCH] D50295: Let GetUnderlyingObject use MemorySSA

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this! In addition to Hal's comments...



================
Comment at: lib/Analysis/ValueTracking.cpp:3500
+          MemoryAccess *MI = MSSA->getMemoryAccess(LI);
+          assert(isa<MemoryUse>(MI) &&
+                 "LoadInst should be MemoryUse node in MemorySSA");
----------------
This isn't always true: MSSA models volatile/atomic (>= monotonic) loads as MemoryDefs. Do we check for this elsewhere in this function?

If so, all of this can be collapsed into `auto *MLoad = cast<MemoryUse>(MSSA->getMemoryAccess(LI));`. If not, please make it a `dyn_cast<MemoryUse>(MSSA->getMemoryAccess(LI))` + null check.


================
Comment at: lib/Analysis/ValueTracking.cpp:3511
+            MemoryDef *MDef = dyn_cast<MemoryDef>(MA);
+            if (MDef->getMemoryInst() != nullptr) {
+              StoreInst *DefI = dyn_cast<StoreInst>(MDef->getMemoryInst());
----------------
Nit: I think it's way more common/somewhat more readable to say `MDef != MSSA->getLiveOnEntry()`.

If any other Def/Use has a null MemoryInst, MSSA is probably broken. :)


Repository:
  rL LLVM

https://reviews.llvm.org/D50295