RFR: 8240220: IdealLoopTree::dump_head predicate printing is broken

[Christian Hagedorn]

Hi Stefan

Looks good to me, thank you for fixing this!

Best regards,
Christian

On 28.02.20 14:21, Nils Eliasson wrote:
> +1
> 
> // Nils
> 
> On 2020-02-28 14:10, Tobias Hartmann wrote:
>> Hi Stefan,
>>
>> good catch! Looks good to me.
>>
>> Best regards,
>> Tobias
>>
>> On 28.02.20 11:49, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> Please review this patch to fix the loop predicate printing in 
>>> IdealLoopTree::dump_head.
>>>
>>> https://cr.openjdk.java.net/~stefank/8240220/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8240220
>>>
>>> In GraphKit::add_predicate you can see the order in which the 
>>> predicates are added:
>>>
>>>      add_predicate_impl(Deoptimization::Reason_predicate, nargs);
>>>      add_predicate_impl(Deoptimization::Reason_profile_predicate, 
>>> nargs);
>>>      add_predicate_impl(Deoptimization::Reason_loop_limit_check, nargs);
>>>
>>> When IdealLoopTree::dump_head looks for these predicates, the code 
>>> has incorrectly swapped the
>>> 'predicate' and 'profile_predicate':
>>>
>>> 2541   Node* predicate = 
>>> PhaseIdealLoop::find_predicate_insertion_point(entry,
>>> Deoptimization::Reason_loop_limit_check);
>>> 2547     entry = PhaseIdealLoop::find_predicate_insertion_point(entry,
>>> Deoptimization::Reason_predicate);
>>> 2554     entry = PhaseIdealLoop::find_predicate_insertion_point(entry,
>>> Deoptimization::Reason_profile_predicate);
>>>
>>> (Note that the predicates are added top-down, while dump_head walks 
>>> them bottom-up)
>>>
>>> The code also uses wrong node when calling skip_loop_predicates. 
>>> Compare the skipping of 'limit_check':
>>>
>>> 2541   Node* predicate = 
>>> PhaseIdealLoop::find_predicate_insertion_point(entry,
>>> Deoptimization::Reason_loop_limit_check);
>>> 2542   if (predicate != NULL ) {
>>> 2543     tty->print(" limit_check");
>>> 2544     entry = PhaseIdealLoop::skip_loop_predicates(entry);
>>>
>>> and see how the skipping of 'predicate' sets entry on line 2547;
>>> 2547     entry = PhaseIdealLoop::find_predicate_insertion_point(entry,
>>> Deoptimization::Reason_predicate);
>>> 2548     if (entry != NULL) {
>>> 2549       tty->print(" predicated");
>>> 2550       entry = PhaseIdealLoop::skip_loop_predicates(entry);
>>> 2551     }
>>>
>>> Before the patch, the 'profile_predicated' and 'predicated' items 
>>> were missing:
>>>    Loop: N344/N304  limit_check counted [0,int),+1 (-1 iters)  has_sfpt
>>>
>>> After the patch, the lines look like this:
>>>    Loop: N344/N304  limit_check profile_predicated predicated counted 
>>> [0,int),+1 (-1 iters)  has_sfpt
>>>
>>> I've only tested this by visually inspecting the output. I don't know 
>>> if there are any existing
>>> tests for this kind of code in the Compiler.
>>>
>>> Thanks,
>>> StefanK
>