RFR: 8263352: assert(use == polladr) failed: the use should be a safepoint polling

[Vladimir Kozlov]

On Fri, 19 Mar 2021 11:19:30 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> I have question for @rwestrel who did strip mining. Looking on nodes I see:
>>  1247  CountedLoop  ===  1247  1246  794  [[ 1247  788  741  738  781  1242 ]] inner stride: 1  strip mined !orig=[1237],[731] !jvms: ComponentSampleModel::<init> @ bci:147 (line 162)
>>  1242  CountedLoopEnd  ===  1247  1241  [[ 1243  794 ]] [lt] P=0.646827, C=6564.000000 !orig=[793] !jvms: ComponentSampleModel::<init> @ bci:144 (line 161)
>>  1243  IfFalse  ===  1242  [[ 1248 ]] #0 !orig=795 !jvms: ComponentSampleModel::<init> @ bci:144 (line 161)
>> 
>>  788  LoadP  ===  1247  659  787  [[ 1248 ]]  @rawptr:BotPTR, idx=Raw; #rawptr:BotPTR (does not depend only on test) !jvms: ComponentSampleModel::<init> @ bci:158 (line 161)
>>  1248  SafePoint  ===  1243  1  784  1  1  788  10  1  1  1  1  1  1  782  [[ 1244 ]]  SafePoint  !orig=783 !jvms: ComponentSampleModel::<init> @ bci:158 (line 161)
>> So why Poll's LoadP node's control edge  is not the same as SafePoint's control edge? Why it points to inner loop head?
>> If we adjust LoadP's control edge to the same node as Safepoint (exit from inner loop), we would not need this special case in match_fill_loop().
>
>> I have question for @rwestrel who did strip mining. Looking on nodes I see:
>> 
>> ```
>>  1247  CountedLoop  ===  1247  1246  794  [[ 1247  788  741  738  781  1242 ]] inner stride: 1  strip mined !orig=[1237],[731] !jvms: ComponentSampleModel::<init> @ bci:147 (line 162)
>>  1242  CountedLoopEnd  ===  1247  1241  [[ 1243  794 ]] [lt] P=0.646827, C=6564.000000 !orig=[793] !jvms: ComponentSampleModel::<init> @ bci:144 (line 161)
>>  1243  IfFalse  ===  1242  [[ 1248 ]] #0 !orig=795 !jvms: ComponentSampleModel::<init> @ bci:144 (line 161)
>> 
>>  788  LoadP  ===  1247  659  787  [[ 1248 ]]  @rawptr:BotPTR, idx=Raw; #rawptr:BotPTR (does not depend only on test) !jvms: ComponentSampleModel::<init> @ bci:158 (line 161)
>>  1248  SafePoint  ===  1243  1  784  1  1  788  10  1  1  1  1  1  1  782  [[ 1244 ]]  SafePoint  !orig=783 !jvms: ComponentSampleModel::<init> @ bci:158 (line 161)
>> ```
>> 
>> So why Poll's LoadP node's control edge is not the same as SafePoint's control edge? Why it points to inner loop head?
>> If we adjust LoadP's control edge to the same node as Safepoint (exit from inner loop), we would not need this special case in match_fill_loop().
> 
> That makes sense but needs to be done explicitly:
> diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp
> index bf5a685960c..42e5b940dc5 100644
> --- a/src/hotspot/share/opto/loopnode.cpp
> +++ b/src/hotspot/share/opto/loopnode.cpp
> @@ -1636,6 +1636,13 @@ bool PhaseIdealLoop::is_counted_loop(Node* x, IdealLoopTree*&loop, BasicType iv_
>          Node* sfpt = sfpt2->clone();
>          sfpt->set_req(0, iffalse);
>          outer_le->set_req(0, sfpt);
> +        Node* polladdr = sfpt->in(TypeFunc::Parms)->clone();
> +        assert(polladdr->is_Load(), "");
> +        Node* new_polladdr = polladdr->clone();
> +        new_polladdr->set_req(0, iffalse);
> +        _igvn.register_new_node_with_optimizer(new_polladdr, polladdr);
> +        set_ctrl(new_polladdr, iffalse);
> +        sfpt->set_req(TypeFunc::Parms, new_polladdr);
>          // When this code runs, loop bodies have not yet been populated.
>          const bool body_populated = false;
>          register_control(sfpt, outer_ilt, iffalse, body_populated);

Yes, that is it.

@rwestrel  why you need first clone?
Node* polladdr = sfpt->in(TypeFunc::Parms)->clone();

I think we can use original one because it is only used to copy notes in  `register_new_node_with_optimizer()`

And do we always have this Load? May be runtime check instead of assert?

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

PR: https://git.openjdk.java.net/jdk/pull/3061