RFR: 8325451: Missed elimination of assertion predicates

[Christian Hagedorn]

On Thu, 8 Feb 2024 06:39:49 GMT, Joshua Cao <duke at openjdk.org> wrote:

> For this example:
> 
> 
> public void blackhole(int[] arr) {
>   for (int i = 0; i < arr.length; ++i) {
>     blackhole(arr[i]);
>   }
> }
> 
> 
> We generate predicates for the lower/upper bound. The lower bound predicate gets optimized away, but the associated assertion predicate remains in the code.
> 
> The issue is that we iterate over list `C->template_assertion_predicate_opaq_node`, and we delete items from it in `_igvn.replace_node(n, n->in(2))`. I was surprised that this actually removes an item from the predicates list. Here is the trace:
> 
> 
> #0  Compile::remove_template_assertion_predicate_opaq (this=0x7fffb5d5a7e0, n=0x7fff84097eb0) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/compile.hpp:776
> #1  0x00007ffff5313c53 in Compile::remove_useless_node (this=0x7fffb5d5a7e0, dead=0x7fff84097eb0) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/compile.cpp:395
> #2  0x00007ffff5cebb95 in PhaseIterGVN::remove_globally_dead_node (this=0x7fffb5d584e0, dead=0x7fff84097eb0) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/phaseX.cpp:1380
> #3  0x00007ffff52241ce in PhaseIterGVN::remove_dead_node (this=0x7fffb5d584e0, dead=0x7fff84097eb0) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/phaseX.hpp:527
> #4  0x00007ffff5cebfc7 in PhaseIterGVN::subsume_node (this=0x7fffb5d584e0, old=0x7fff84097eb0, nn=0x7fff8408fb80) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/phaseX.cpp:1429
> #5  0x00007ffff50045bf in PhaseIterGVN::replace_node (this=0x7fffb5d584e0, old=0x7fff84097eb0, nn=0x7fff8408fb80) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/phaseX.hpp:539
> #6  0x00007ffff5aec060 in PhaseIdealLoop::eliminate_useless_template_assertion_predicates (this=0x7fffb5d58120, useful_predicates=...) at /local/home/joshcao/pea/jdk/src/hotspot/share/opto/loopnode.cpp:4366
> 
> 
> We can fix this by iterating in reverse over the linked list.
> 
> Caveat: this patch only affects the first test `TestPredicateBasic::basic()`. Adding the other tests for completeness and can potentially be improved in other work.

Good catch! The complete fix for Assertion Predicates [JDK-8288981](https://bugs.openjdk.org/browse/JDK-8288981) will remove this code here and do the cleanup differently. But for now, I think it's good to fix it.

src/hotspot/share/opto/loopnode.cpp line 4367:

> 4365:     if (!useful_predicates.member(n)) { // not in the useful list
> 4366:       _igvn.replace_node(n, n->in(2));
> 4367:     }

I think we should keep the name `opaque4`.

-------------

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17767#pullrequestreview-1869483807
PR Review Comment: https://git.openjdk.org/jdk/pull/17767#discussion_r1482552828