[PATCH] D65338: [MemorySSA] Extend allowed behavior for simplified instructions.
George Burgess IV via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 30 11:08:21 PDT 2019
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
More information about the llvm-commits
mailing list