Cleanup dead phis in SILMem2Reg for OSSA #38265
Conversation
|
@swift-ci test |
meg-gupta
force-pushed
the
alternatemem2regfix2
branch
2 times, most recently
from
0c7b3eb to
1268709
Compare
|
@swift-ci test |
|
Build failed |
|
@swift-ci clean test Linux Platform |
|
|
||
| // If already visited, don't explore further. | ||
| if (visitedPhis.contains(phi)) | ||
| return true; |
There was a problem hiding this comment.
return true looks wrong here. E.g.
bb1(%phi1):
br bb2(%phi1)
bb2(%phi2): // not dead!
// use %phi2
Let's say isUnnecessaryPhi is called for %phi1 first - and returns false (which is correct). When isUnnecessaryPhi is then called for %phi2, which is already in visitedPhis, it will return true.
I don't think you can solve the dead-phi-removal problem by walking the def-use graph in "forward" direction.
Instead, you can mark all used phi arguments and propagate those flags backward. Like it's done in DeadCodeElimination.
There was a problem hiding this comment.
I clear the visitedPhis set in the main loop before successive calls. It's not actually a cache. Going to look at DCE now.
There was a problem hiding this comment.
ok, I didn't see that. But then the problem is that the algorithm is quadratic - in case of long phi-chains.
BTW, as you can have only one phi argument per block, you could also use a BasicBlockSet: visitedBlock instead of visitedPhis. With that you don't need to keep the set around for multiple isUnnecessaryPhi calls for performance reasons. You can just declare it inside isUnnecessaryPhi
There was a problem hiding this comment.
I am following the approach of backward propagating like DCE now. Thanks for pointing me to that. I did leave the new livePhis as a SmallPtrSet for readability reasons.
meg-gupta
force-pushed
the
alternatemem2regfix2
branch
from
1268709 to
5966ed4
Compare
Mem2Reg creates phis proactively that may be unnecessary. Unnecessary phis are those without uses or operands to other proactive phis that are unnecessary. Even though this does not translate to a real issue at runtime, we can see ownership verification errors. This PR identifies such phis and deletes them.
With such alloc_stack we can pass over-consumed values to a proactively added phi. Even though such a SIL does not have issues at runtime, they raise an ownership verification error.
meg-gupta
force-pushed
the
alternatemem2regfix2
branch
from
5966ed4 to
dc57d6b
Compare
|
@swift-ci test |
|
Build failed |
|
lldb test failed.. |
|
@eeckstein Thanks for the review! |
|
Build failed |
|
@swift-ci clean test Linux Platform |
Mem2Reg creates phis proactively that may be unnecessary.
Unnecessary phis are those without uses or operands to other
proactive phis that are unnecessary.
Even though this does not translate to a real issue at runtime, we can
see ownership verification errors. This PR identifies such phis and deletes them.
Fixes rdar://79349413