RFR: 8260653: Unreachable nodes keep speculative types alive
Locked
RFR: 8260653: Unreachable nodes keep speculative types alive
 
|
8260653: Unreachable nodes keep speculative types alive
------------- Commit messages: - Remove useless nodes Changes: https://git.openjdk.java.net/jdk/pull/2606/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260653 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606 PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive
|
|
On Wed, 17 Feb 2021 10:27:08 GMT, Nils Eliasson <[hidden email]> wrote:
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail. > > This problem will not cause crashes in production - it is only a sanity test. > > I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. > > When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. > > A big thank you to Dean Long how created the reproducer for this bug. > > Please review. One reasonable alternative to adding one more pass of PhaseRemoveUseless is filtering out all non-reachable nodes from NodeHash::check_no_speculative_types. The unreachable nodes only live a short while before being removed in the renumbering phase. ------------- PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v2]
|
|
In reply to this post by Nils Eliasson-2
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
> > This problem will not cause crashes in production - it is only a sanity test. > > I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. > > When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. > > A big thank you to Dean Long how created the reproducer for this bug. > > Please review. Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: Check if node is live before assert ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2606/files - new: https://git.openjdk.java.net/jdk/pull/2606/files/5f279179..82ab1066 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=00-01 Stats: 10 lines in 2 files changed: 6 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606 PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v2]
|
|
On Wed, 17 Feb 2021 14:35:59 GMT, Nils Eliasson <[hidden email]> wrote:
>> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail. >> >> This problem will not cause crashes in production - it is only a sanity test. >> >> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. >> >> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. >> >> A big thank you to Dean Long how created the reproducer for this bug. >> >> Please review. > > Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: > > Check if node is live before assert Looks good. src/hotspot/share/opto/phaseX.cpp line 341: > 339: n != sentinel_node && > 340: n->is_Type() && > 341: n->outcnt() > 0 && `live_nodes.member(n)` makes ` n->outcnt() > 0` redundant, doesn't it? ------------- Marked as reviewed by vlivanov (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v3]
|
|
In reply to this post by Nils Eliasson-2
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
> > This problem will not cause crashes in production - it is only a sanity test. > > I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. > > When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. > > A big thank you to Dean Long how created the reproducer for this bug. > > Please review. Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: Remove outcnt check ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2606/files - new: https://git.openjdk.java.net/jdk/pull/2606/files/82ab1066..ac23dad8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606 PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v2]
|
|
In reply to this post by Vladimir Ivanov-2
On Wed, 17 Feb 2021 16:34:43 GMT, Vladimir Ivanov <[hidden email]> wrote:
>> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: >> >> Check if node is live before assert > > src/hotspot/share/opto/phaseX.cpp line 341: > >> 339: n != sentinel_node && >> 340: n->is_Type() && >> 341: n->outcnt() > 0 && > > `live_nodes.member(n)` makes ` n->outcnt() > 0` redundant, doesn't it? Yes it does. I've updated the patch. ------------- PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v3]
|
|
In reply to this post by Nils Eliasson-2
On Wed, 17 Feb 2021 18:16:02 GMT, Nils Eliasson <[hidden email]> wrote:
>> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail. >> >> This problem will not cause crashes in production - it is only a sanity test. >> >> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. >> >> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. >> >> A big thank you to Dean Long how created the reproducer for this bug. >> >> Please review. > > Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: > > Remove outcnt check Otherwise looks good. src/hotspot/share/opto/phaseX.cpp line 338: > 336: for (uint i = 0; i < max; ++i) { > 337: Node *n = at(i); > 338: if(n != NULL && Missing whitespace between `if` and `(` ------------- Marked as reviewed by thartmann (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v4]
|
|
In reply to this post by Nils Eliasson-2
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
> > This problem will not cause crashes in production - it is only a sanity test. > > I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. > > When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. > > A big thank you to Dean Long how created the reproducer for this bug. > > Please review. Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: Add missing ws ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2606/files - new: https://git.openjdk.java.net/jdk/pull/2606/files/ac23dad8..ae048cbc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606 PR: https://git.openjdk.java.net/jdk/pull/2606 |
Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v3]
|
|
In reply to this post by Tobias Hartmann-3
On Thu, 18 Feb 2021 08:10:08 GMT, Tobias Hartmann <[hidden email]> wrote:
>> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision: >> >> Remove outcnt check > > Otherwise looks good. Thanks for the reviews Vladimir and Tobias! ------------- PR: https://git.openjdk.java.net/jdk/pull/2606 |
Integrated: 8260653: Unreachable nodes keep speculative types alive
|
|
In reply to this post by Nils Eliasson-2
On Wed, 17 Feb 2021 10:27:08 GMT, Nils Eliasson <[hidden email]> wrote:
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail. > > This problem will not cause crashes in production - it is only a sanity test. > > I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left. > > When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive. > > A big thank you to Dean Long how created the reproducer for this bug. > > Please review. This pull request has now been integrated. Changeset: 3a21e1df Author: Nils Eliasson <[hidden email]> URL: https://git.openjdk.java.net/jdk/commit/3a21e1df Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod 8260653: Unreachable nodes keep speculative types alive Reviewed-by: vlivanov, thartmann ------------- PR: https://git.openjdk.java.net/jdk/pull/2606 |
«
Return to OpenJDK Hotspot Compiler Development List
|
1 view|%1 views
| Free forum by Nabble | Edit this page |