[PATCH] D65338: [MemorySSA] Extend allowed behavior for simplified instructions.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: lib/Analysis/MemorySSA.cpp:1690
                                                MemoryAccess *Definition,
                                                const MemoryUseOrDef *Template) {
   assert(!isa<PHINode>(I) && "Cannot create a defined access for a PHI");
----------------
This new optionality is surprising to me, and I think something that's not generally desirable for users of this function. Would it make sense to refactor this slightly into something like `tryCreateDefinedAccess` and `createDefinedAccess`? (the `try` version being the body of this function without the `assert(NewAccess != nullptr)`, and the `create` literally just being `try` + the `assert`)

Alternatively, I'd be happy with an added `bool CreationMustSucceed = true` param that stands in for the new `if (Template)` conditional, if you feel that's better.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:538
             CloneWasSimplified ? nullptr : MUD);
-        MSSA->insertIntoListsForBlock(NewUseOrDef, NewBB, MemorySSA::End);
+        if (NewUseOrDef)
+          MSSA->insertIntoListsForBlock(NewUseOrDef, NewBB, MemorySSA::End);
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > `NewUseOrDef` should always be non-null, no?
> No, this can now be null.
> Say we have a simplified instruction such that the clone is not a `MemoryAccess`. We pass the `Template` as nullptr to `createDefinedAccess`, and inside it we call `createNewAccess` which will return nullptr. Since we didn't pass a `Template`, the assertion won't hit, i.e. once the instruction is simplified we cannot rely on the Template info, and we can no longer assume we will actually create a MemoryAccess.
> Does this make sense?
Oh, right. I was looking at my source without the patch applied when thinking about this question. This is what I get for reviewing CLs late -- sorry. :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65338