RFR(M): 8223051: support loops with long (64b) trip counts

Roland Westrelin rwestrel at redhat.com
Thu Aug 20 16:05:39 UTC 2020


Hi Vladimir,

Thanks for taking a look at this.

> ===============
> src/hotspot/share/opto/callnode.cpp:
>
>    // If you have back to back safepoints, remove one
>     if( in(TypeFunc::Control)->is_SafePoint() )
>       return in(TypeFunc::Control);
>
> -  if( in(0)->is_Proj() ) {
> +  // Transforming long counted loops requires a safepoint node. Do not
> +  // eliminate a safepoint until loop opts are over.
> +  if (in(0)->is_Proj() && !phase->C->major_progress()) {
>
> Can you elaborate on this a bit? Why elimination of back-to-back 
> safepoints cause problems during new transformation? Is it because you 
> need specifically a SafePoint because CallNode doesn't fit?

The issue is with a call followed by a SafePointNode. A call captures
the state before the call but we would need the state after the call
otherwise on a deoptimization we would re-executed the call.

> ===============
> src/hotspot/share/opto/loopnode.cpp:
>
> +void PhaseIdealLoop::add_empty_predicate(Deoptimization::DeoptReason 
> reason, Node* inner_head, IdealLoopTree* loop, SafePointNode* sfpt) {
>
> Nothing actionable at the moment, but it's unfortunate to see more and 
> more code being duplicated from GraphKit. I wish there were a way to 
> share implementation between GraphKit, PhaseIdealLoop, and 
> PhaseMacroExpand.

Actually, there might be a way. In the valhalla repo, Tobias pushed a
change to GraphKit so it's possible to build one with an igvn
argument. So we could do this:

    JVMState* jvms = cloned_sfpt->jvms()->clone_shallow(C);
    SafePointNode* map = cloned_sfpt->clone()->as_SafePoint();
    map->set_jvms(jvms);
    jvms->set_map(map);
    GraphKit kit(jvms, &_igvn);
    kit.set_control(inner_head->in(LoopNode::EntryControl));

    kit.add_empty_predicates(0);

    _igvn.replace_input_of(inner_head, LoopNode::EntryControl, kit.control());
    _igvn.remove_dead_node(map);

instead of:

    if (UseLoopPredicate) {
      add_empty_predicate(Deoptimization::Reason_predicate, inner_head, outer_ilt, cloned_sfpt);
    }
    if (UseProfiledLoopPredicate) {
      add_empty_predicate(Deoptimization::Reason_profile_predicate, inner_head, outer_ilt, cloned_sfpt);
    }
    add_empty_predicate(Deoptimization::Reason_loop_limit_check, inner_head, outer_ilt, cloned_sfpt);

and the new PhaseIdealLoop::add_empty_predicate() wouldn't be needed
anymore.

One thing to consider is that new nodes for predicates are added by
GraphKit now and they are not registered with PhaseIdealLoop. It may not
be a problem because peeling sets major_progress so no further loop opts
will be applied in this round.

Anyway, if we wanted to pursue this further, I think it would make sense
to push Tobias' patch first.

What do you think?

Roland.



More information about the hotspot-compiler-dev mailing list