[LVA] Support access instructions in DCE. #31133
Conversation
zoecarver
force-pushed
the
lva/dead-access
branch
from
0c80a1f to
167d992
Compare
|
|
||
| if (isa<EndAccessInst>(I)) | ||
| return I->getOperand(0)->getSingleUse() == nullptr; | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, you're right, that would be better. Will do.
| } | ||
| continue; | ||
| } | ||
| if (auto access = dyn_cast<BeginAccessInst>(&I)) { |
There was a problem hiding this comment.
begin_access is marked as having side effects so, I still have to have this bit of logic.
There was a problem hiding this comment.
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().
| @@ -258,6 +258,19 @@ void DCE::markLive(SILFunction &F) { | |||
| } | |||
| continue; | |||
| } | |||
| if (isa<EndAccessInst>(&I)) { | |||
| if (auto *access = dyn_cast<BeginAccessInst>(I.getOperand(0))) { | |||
There was a problem hiding this comment.
The operand of an end_access must be a begin_access. You can use EndAccessInst::getBeginAccess() to get it.
There was a problem hiding this comment.
Good to know. My thinking was that there could be a phi argument or something. Is that not valid?
There was a problem hiding this comment.
No, it must be a begin_access
| } | ||
| continue; | ||
| } | ||
| if (auto access = dyn_cast<BeginAccessInst>(&I)) { |
There was a problem hiding this comment.
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
force-pushed
the
lva/dead-access
branch
from
6d08cb9 to
4d4771f
Compare
| @@ -40,6 +40,11 @@ namespace { | |||
| // FIXME: Reconcile the similarities between this and | |||
| // isInstructionTriviallyDead. | |||
| static bool seemsUseful(SILInstruction *I) { | |||
| // We have a reverse dependency for end_access. If there are no uses of a | |||
| // begin_access, both will be removed. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough. Done.
Simple check for the following pattern: x = begin_access end_access x
zoecarver
force-pushed
the
lva/dead-access
branch
from
4d4771f to
e22e7bc
Compare
|
@swift-ci please smoke test and merge |
A simple check for the following pattern: