This is an archive of the discontinued LLVM Phabricator instance.

[DSE][NFC] Introduce "doesn't overwrite" return code for isOverwrite
ClosedPublic

Authored by ebrevnov on Jun 29 2021, 3:21 AM.

Details

Summary

Add OR_None code to indicate that there is no overwrite. This has no any effect for current uses but will be used in one of the next patches building support for PHI translation.

Diff Detail

Event Timeline

ebrevnov created this revision.Jun 29 2021, 3:21 AM
ebrevnov requested review of this revision.Jun 29 2021, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 3:21 AM
xbolva00 added inline comments.
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1058

Adjust comment

ebrevnov updated this revision to Diff 355189.Jun 29 2021, 5:36 AM

Fixed comments + formatting

ebrevnov updated this revision to Diff 355191.Jun 29 2021, 5:48 AM

Minor update

fhahn added a comment.Sep 6 2021, 3:56 AM

Looks good in general, but I couldn't find a use in the linked patches. Could you update the description with more details about the motivation, when/how this will be used.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1457

The comment here seems slightly inaccurate and I think it is more obvious after this change; we skip if we can't prove that there is a (partial) overwrite. Might be good to adjust the comment.

1468

This seems unrelated to the patch?

Looks good in general, but I couldn't find a use in the linked patches. Could you update the description with more details about the motivation, when/how this will be used.

The thing is that I had to remove depending changes from the list since I didn't manage to come up with a regression test. Looks like it is not that critical and we can return to it once main functionality settles down. So let's skip this one for now as well if you don't mind :-)

fhahn requested changes to this revision.Sep 17 2021, 1:03 AM

Looks good in general, but I couldn't find a use in the linked patches. Could you update the description with more details about the motivation, when/how this will be used.

The thing is that I had to remove depending changes from the list since I didn't manage to come up with a regression test. Looks like it is not that critical and we can return to it once main functionality settles down. So let's skip this one for now as well if you don't mind :-)

Sounds good to me. Marking as changes requested so it is gone from the review queue for now.

This revision now requires changes to proceed.Sep 17 2021, 1:03 AM
fhahn added a comment.Oct 31 2021, 9:46 AM

Looks good in general, but I couldn't find a use in the linked patches. Could you update the description with more details about the motivation, when/how this will be used.

The thing is that I had to remove depending changes from the list since I didn't manage to come up with a regression test. Looks like it is not that critical and we can return to it once main functionality settles down. So let's skip this one for now as well if you don't mind :-)

Sounds good to me. Marking as changes requested so it is gone from the review queue for now.

I've encountered another case where OW_None would be helpful: D112313.

@ebrevnov I think it would be very useful to get this patch in soonish to unblock D112313 & following. I put up a variation of this patch as D112312 with a small addition: returning OW_None if both accesses do not alias. It would be good to include those here as well.

fhahn added a comment.Nov 9 2021, 3:24 AM

reverse ping :)

fhahn accepted this revision.Nov 22 2021, 5:24 AM

LGTM, thanks!

This revision is now accepted and ready to land.Nov 22 2021, 5:24 AM
This revision was landed with ongoing or failed builds.Nov 23 2021, 2:11 AM
This revision was automatically updated to reflect the committed changes.