Skip to content

Conversation

@zoecarver
Copy link
Contributor

A simple check for the following pattern:

x = begin_access
end_access x


if (isa<EndAccessInst>(I))
return I->getOperand(0)->getSingleUse() == nullptr;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work.
But a better approach would be to add an end_access with a reverse dependency to begin_access.
Usually an instruction is dead of all its uses are dead. With a reverse dependency an instruction is dead if it's operand is dead. And that's what we want for end_access: it should be removed if the its operand (= the begin_access) is dead.

See the handling of cond_fail and fix_lifetime in DCE as examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, that would be better. Will do.

@zoecarver zoecarver requested a review from eeckstein May 9, 2020 04:48
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

begin_access is marked as having side effects so, I still have to have this bit of logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logic looks wrong. It should be sufficient to exclude begin_access in seemsUseful().
If there is a use (other than end_access), the begin_access will be marked as life in propagateLiveness().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operand of an end_access must be a begin_access. You can use EndAccessInst::getBeginAccess() to get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. My thinking was that there could be a phi argument or something. Is that not valid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it must be a begin_access

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logic looks wrong. It should be sufficient to exclude begin_access in seemsUseful().
If there is a use (other than end_access), the begin_access will be marked as life in propagateLiveness().

@zoecarver zoecarver requested a review from eeckstein May 12, 2020 03:12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Can you change this comment to something like: "begin_access is defined to have side effects, but this is not relevant for DCE."
Because this is the reason why you have to handle begin_access here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Done.

Simple check for the following pattern:
x = begin_access
end_access x
@zoecarver zoecarver requested a review from eeckstein May 12, 2020 06:20
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 0560f8c into swiftlang:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants