[PATCH] D55944: [MemorySSA] Extend the clobber walker with the option to skip the starting access.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this! I like it when patches are small and split up like this one :)

I'd rather not check in dynamically dead code, though.  Can we please hold off on committing this until the patch that makes this un-dead is approved and such?



================
Comment at: lib/Analysis/MemorySSA.cpp:625
+      bool ReplacedStopWhere = false;
+      if (Query->SkipSelfAccess && Query->OriginalAccess &&
+          Node.Loc == Query->StartingLoc &&
----------------
Is it sensible to have SkipSelfAccess with Query->OriginalAccess == null?


================
Comment at: lib/Analysis/MemorySSA.cpp:628
+          Node.Last->getBlock() == Query->OriginalAccess->getBlock()) {
+        assert(isa<MemoryDef>(Query->OriginalAccess));
+        RealStopWhere = Query->OriginalAccess;
----------------
Can we move this assert where we plan to set `Query->SkipSelfAccess` to true?

That way, it'll fire 100% of the time if there's a bug, rather than only if we reach this piece of code

(I realize that'll be in the follow-up patch. Still. :) )


================
Comment at: lib/Analysis/MemorySSA.cpp:650
         // walking.
-        NewPaused.push_back(PathIndex);
+        if (!ReplacedStopWhere)
+          NewPaused.push_back(PathIndex);
----------------
Please add a short comment explaining why this if exists, since I feel like I'm likely to forget the trickiness we're doing here, and it would help any future readers of the code. 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55944/new/

https://reviews.llvm.org/D55944