This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Extend allowed behavior for simplified instructions.
ClosedPublic

Authored by asbirlea on Jul 26 2019, 10:13 AM.

Details

Summary

LoopRotate may simplify instructions, leading to the new instructions not having memory accesses created for them.
Allow this behavior, by allowing the new access to be null when the template is null, and looking upwards for the proper defined access when dealing with simplified instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Jul 26 2019, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 10:13 AM
Herald added subscribers: Prazek, jlebar. · View Herald Transcript

Thanks for this! Two nits and a question

lib/Analysis/MemorySSAUpdater.cpp
487 ↗(On Diff #211961)

nit: if it's straightforward to turn this into a static helper, please do so.

538 ↗(On Diff #211961)

NewUseOrDef should always be non-null, no?

test/Analysis/MemorySSA/loop-rotate-simplified-clone.ll
2 ↗(On Diff #211961)

; REQUIRES: asserts

asbirlea updated this revision to Diff 212383.Jul 30 2019, 10:51 AM
asbirlea marked 4 inline comments as done.

Address comments.

lib/Analysis/MemorySSAUpdater.cpp
538 ↗(On Diff #211961)

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?

lib/Analysis/MemorySSA.cpp
1690 ↗(On Diff #212383)

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.

lib/Analysis/MemorySSAUpdater.cpp
538 ↗(On Diff #211961)

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. :)

asbirlea updated this revision to Diff 212392.Jul 30 2019, 11:19 AM

Added optional bool CreationMustSucceed.

asbirlea marked 2 inline comments as done.Jul 30 2019, 11:19 AM
This revision is now accepted and ready to land.Jul 30 2019, 11:29 AM
This revision was automatically updated to reflect the committed changes.