RFR: 8350756: C2 SuperWord Multiversioning: remove useless slow loop when the fast loop disappears [v2]

[Christian Hagedorn]

On Tue, 4 Mar 2025 09:26:39 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/loopnode.cpp line 2743:
>> 
>>> 2741:   IfTrueNode* before_predicates = predicates.entry()->isa_IfTrue();
>>> 2742:   if (before_predicates != nullptr &&
>>> 2743:       before_predicates->in(0)->is_If() &&
>> 
>> Since this is after IGVN and we have not applied any loop transformations in this round, yet, shouldn't this always hold since we check that we have a `IfTrue` as child?
>
> You are probably right. But the method `CountedLoopNode::find_multiversion_if_from_multiversion_fast_main_loop` could be used from other contexts later. So I'd rather not make such assumptions. What do you think?

I don't think that the input of an `IfTrue` is anything else than an `IfNode` (or a subclass) while being outside of IGVN. Otherwise, the graph is broken. Moreover, if we decide to call this method from IGVN at some point, we face another problem that predicate iteration is not safe during IGVN - and we probably do not want to make it safe if there is not a very strong reason for it.

>> src/hotspot/share/opto/loopopts.cpp line 794:
>> 
>>> 792:     return nullptr;
>>> 793:   }
>>> 794:   if (bol->Opcode() != Op_Bool) {
>> 
>> Could we use `!bol->is_Bool()` here?
>
> It would be equivalent now, since nobody inherits from `BoolNode`. But if someone ever inherits from it, then this condition would be weaker. Also: I just copied the condition.
> 
> I can do it if you still want me to do it, I'm on the fence with this one ;)

I'm not sure if we were ever to extend `BoolNode`. And if so, I think implementing this requires to check all the `is_Bool()` conditions in our code base anyways - which I expect we have quite a lot of them. And it could be that a subclass would also allow to apply a cmove transformation. So, given that it's unlikely, I would be more inclined to change it to `is_Bool()`. But it's your call. I'm fine with both :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23865#discussion_r1979062802
PR Review Comment: https://git.openjdk.org/jdk/pull/23865#discussion_r1979061916