This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Handle masked stores
ClosedPublic

Authored by kparzysz on Sep 9 2020, 8:41 AM.

Details

Reviewers
fhahn
asbirlea
Summary

Recognize masked stores and eliminate dead ones.

Diff Detail

Event Timeline

kparzysz created this revision.Sep 9 2020, 8:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 8:41 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
kparzysz requested review of this revision.Sep 9 2020, 8:41 AM
This revision is now accepted and ready to land.Sep 9 2020, 11:16 AM
fhahn added a comment.Sep 9 2020, 11:35 AM

Could you also add a test that uses the MemorySSA DSE implementation (-enable-dse-memoryssa), which will become the default very soon.

kparzysz closed this revision.Sep 9 2020, 11:35 AM

Committed in https://reviews.llvm.org/rG81ff2d30a900. Forgot to update the commit message. :(

@fhahn Sure, let me do it now.

Hm, it fails with MemorySSA. Let me figure this out...

I disabled MSSA in this testcase so it won't fail when you switch (db7defd9bab7527ec1d0ed3fc62b379a9adf0971). I'll fix it in the meantime.

Could you please prioritize figuring out why the test fails with DSE+MSSA? It's important to have this clarified before the flag flip.

fhahn added a comment.Sep 9 2020, 12:25 PM

Could you please prioritize figuring out why the test fails with DSE+MSSA? It's important to have this clarified before the flag flip.

I suspect it needs adding support for masked_store to getLocForWriteEx. It fails as in it is a missed optimization, right?

Yes, it fails to remove the store.

Still investigating. Changing getLocForWriteEx doesn't fix it.