RFR: 8339733: C2: some nodes can have incorrect control after do_range_check()

[Christian Hagedorn]

On Mon, 9 Sep 2024 08:27:20 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> PhaseIdealLoop::do_range_check() sets the control of the new pre and
> main limits to be the entry control of the pre loop but it eliminates
> all conditions whose parameters are invariant in the main loop. Most
> of the time they are also invariant in the pre loop but that's not
> guaranteed. It does happen sometimes that those parameters are pinned
> in the pre loop. In that case, PhaseIdealLoop::do_range_check() sets
> wrong controls.
> 
> This doesn't cause any issue today AFAICT.
> 
> Also, this seems to be a typo in PhaseIdealLoop::insert_pre_post_loops():
> 
> 
> pre_head->in(0)
> 
> 
> is `pre_head`. I fixed that one too.

Overall, the fix looks good! A few suggestions.

src/hotspot/share/opto/loopTransform.cpp line 1687:

> 1685: 
> 1686:   register_new_node(pre_limit, pre_head->in(LoopNode::EntryControl));
> 1687:   register_new_node(pre_opaq , pre_head->in(LoopNode::EntryControl));

Good catch, I've looked at this code many times before but never noticed that.

src/hotspot/share/opto/loopTransform.cpp line 2793:

> 2791:   // to not ever trip end tests
> 2792:   Node *main_limit = cl->limit();
> 2793:   Node* main_limit_c = get_ctrl(main_limit);

There seems to be some mix between using `_c` and `_ctrl` as postfix. Should we go with `_ctrl` as postfix everywhere to make it more explicit?

src/hotspot/share/opto/loopTransform.cpp line 3095:

> 3093:   set_ctrl(pre_opaq, new_limit_ctrl);
> 3094:   set_ctrl(pre_end->in(1), new_limit_ctrl);
> 3095:   set_ctrl(pre_end->cmp_node(), new_limit_ctrl);

Just for better readability, I suggest to flip those such that it matches the top down order opaque -> cmp -> bool.
Suggestion:

  set_ctrl(pre_end->cmp_node(), new_limit_ctrl);
  set_ctrl(pre_end->in(1), new_limit_ctrl);

src/hotspot/share/opto/loopTransform.cpp line 3139:

> 3137:   set_ctrl(opqzm, new_limit_ctrl);
> 3138:   set_ctrl(iffm->in(1), new_limit_ctrl);
> 3139:   set_ctrl(iffm->in(1)->in(1), new_limit_ctrl);

Same here (flip)
Suggestion:

  set_ctrl(iffm->in(1)->in(1), new_limit_ctrl);
  set_ctrl(iffm->in(1), new_limit_ctrl);

src/hotspot/share/opto/loopTransform.cpp line 3143:

> 3141: 
> 3142: // Adjust control for node and its inputs (and inputs of its inputs) to be above the pre end
> 3143: void PhaseIdealLoop::ensure_node_and_inputs_are_above_pre_end(CountedLoopEndNode* pre_end, Node* node, Node*& control) {

`control` should be the `ctrl` of `node` here, right (i.e. `control == get_ctrl(node)`)? Maybe we can assert that. But thinking about it, would it hurt to just call `get_ctrl(node)` again here and remove `control` as parameter? Then we can just return the new control and do 

offset_c = ensure_node_and_inputs_are_above_pre_end(pre_end, offset);

at the call-site.

src/hotspot/share/opto/loopTransform.cpp line 3157:

> 3155:     assert(is_dominator(compute_early_ctrl(n, get_ctrl(n)), pre_end), "node pinned on loop exit test?");
> 3156:     set_ctrl(n, control);
> 3157:     for (uint j = 0; j < n->req(); ++j) {

For consistency:
Suggestion:

    for (uint j = 0; j < n->req(); j++) {

src/hotspot/share/opto/loopnode.hpp line 1188:

> 1186: 
> 1187:   Node* dominated_node(Node* c1, Node* c2) {
> 1188:     return is_dominator(c1, c2) ? c2 : c1;

Should we also assert here that `is_dominator(c2, c1)`?

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

PR Review: https://git.openjdk.org/jdk/pull/20908#pullrequestreview-2295482534
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753130481
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753131025
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753182663
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753194223
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753168617
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753163523
PR Review Comment: https://git.openjdk.org/jdk/pull/20908#discussion_r1753138453