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

[Stefan Karlsson]

Thanks, Christian.

StefanK

On 2020-02-28 14:45, Christian Hagedorn wrote:
> 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
>>