RFR: 8325451: Missed elimination of assertion predicates [v2]

[Paul Hohensee]

On Thu, 8 Feb 2024 22:28:25 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.
>
> Joshua Cao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - rename n->opaque4
>  - Merge branch 'master' into linkedlist
>  - 8325451: Missed elimination of assertion predicates

Marked as reviewed by phh (Reviewer).

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

PR Review: https://git.openjdk.org/jdk/pull/17767#pullrequestreview-1873335163