SIL: fix problems in findJointPostDominatingSet and some refactoring #35684
Conversation
|
@swift-ci test |
eeckstein
changed the title
eeckstein
changed the title
|
Build failed |
eeckstein
force-pushed
the
fix-find-jpds
branch
from
409c82a to
80dbbfc
Compare
lib/SIL/Utils/BasicBlockUtils.cpp
Outdated
| @@ -390,7 +390,15 @@ void DeadEndBlocks::compute() { | |||
| // Post Dominance Set Completion Utilities | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| void JointPostDominanceSetComputer::findJointPostDominatingSet( | |||
| static bool endsInUnreachable(SILBasicBlock *block) { | |||
There was a problem hiding this comment.
Can you refactor out the immediate logic related to this in DeadEndBlocks (maybe into a helper static function?) and use that here so we have one implementation.
There was a problem hiding this comment.
Also why not just use dead end blocks here? I want @atrick to also look at this.
There was a problem hiding this comment.
ok, I moved it to a static utility function in DeadEndBlocks.
Also why not just use dead end blocks here?
Because it causes the problems I try to fix. E.g. in case the original dominatingBlock is already a dead-end block.
1. dead-end blocks (= blocks which eventually end up in an unreachable): We cannot just ignore all dead-end blocks. This causes crashes for some corner cases (see the "infinite_loop_and_unreachable" test case). Instead just handle the common case of a simple single dead-end block - like we do in DestroyHoisting. For other (more complex) dead-end control flows, the analysis is not incorrect. In worst case we end up inserting a not-needed destroy instruction. 2. sortUnique I restructured the code a bit so that sortUnique is not needed anymore. sortUnique on pointer arrays can result in non-deterministic behavior. 3. lower_bound Also, using lower_bound on a vector is not good in this function, because it can result in quadratic behavior. Though, in practice, there are only very few elements in the vector. So it's more a theoretical thing. The restructuring made the code a bit simpler, e.g. beside the worklist, no other vectors are needed anymore.
Instead make `findJointPostDominatingSet` a stand-alone function. There is no need to keep the temporary SmallVector alive across multiple calls of findJointPostDominatingSet for the purpose of re-using malloc'ed memory. The worklist usually contains way less elements than its small size.
eeckstein
force-pushed
the
fix-find-jpds
branch
from
80dbbfc to
f19f9b0
Compare
|
@swift-ci test |
|
@swift-ci test windows platform |
|
ErikE and I were talking about some other changes here. Around transitive dead end blocks. The implementation change here will be more conservative. I wouldn't be surprised by using the dead end blocks utility we can get a more aggressive result but given that this is fixing a real issue. I want to merge it/iterate later. |
|
@swift-ci test windows platform |
|
Some sort of problem in the windows build that isn't from this (has something to do with parsing or the system itself). Merging! |
We cannot just ignore all dead-end blocks. This causes crashes for some corner cases (see the "infinite_loop_and_unreachable" test case). Instead just handle the common case of a simple single dead-end block - like we do in DestroyHoisting.
For other (more complex) dead-end control flows, the analysis is not incorrect. In worst case we end up inserting a not-needed destroy instruction.
I restructured the code a bit so that sortUnique is not needed anymore. sortUnique on pointer arrays can result in non-deterministic behavior.
Also, using lower_bound on a vector is not good in this function, because it can result in quadratic behavior. Though, in practice, there are only very few elements in the vector. So it's more a theoretical thing.
The restructuring made the code a bit simpler, e.g. beside the worklist, no other vectors are needed anymore.
Also in this PR: remove the JointPostDominanceSetComputer helper struct.
Instead make
findJointPostDominatingSeta stand-alone function.There is no need to keep the temporary SmallVector alive across multiple calls of findJointPostDominatingSet for the purpose of re-using malloc'ed memory. The worklist usually contains way less elements than its small size.