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