RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long) [v11]

[Roland Westrelin]

On Thu, 13 Feb 2025 16:43:22 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> @galderz How sure are that intrinsifying directly is really the right approach?
>> 
>> Maybe the approach via `PhaseIdealLoop::conditional_move` where we know the branching probability is a better one. Though of course knowing the branching probability is no perfect heuristic for how good branch prediction is going to be, but it is at least something.
>> 
>> So I'm wondering if there could be a different approach that sees all the wins you get here, without any of the regressions?
>> 
>> If we are just interested in better vectorization: the current issue is that the auto-vectorizer cannot handle CFG, i.e. we do not yet do if-conversion. But if we had if-conversion, then the inlined CFG of min/max would just be converted to vector CMove (or vector min/max where available) at that point. We can take the branching probabilities into account, just like `PhaseIdealLoop::conditional_move` does - if that is necessary. Of course if-conversion is far away, and we will encounter a lot of issues with branch prediction etc, so I'm scared we might never get there - but I want to try ;)
>> 
>> Do we see any other wins with your patch, that are not due to vectorization, but just scalar code?
>
>> Do we see any other wins with your patch, that are not due to vectorization, but just scalar code?
> 
> I think there are some. 
> 
> The current transformation from the parsed version of min/max to a conditional move to a `Max`/`Min` node depends on the conditional move transformation which has its own set of heuristics and while it happens on simple test cases, that's not necessarily the case on all code shapes. I don't think we want to trust it too much.
> 
> With the intrinsic, the type of the min or max can be narrowed down in a way it can't be whether the code includes control flow or a conditional move. That in turn, once types have propagated, could cause some constant to appear and could be a significant win.
> 
> The `Min`/`Max` nodes are floating nodes. They can hoist out of loop and common reliably in ways that are not guaranteed  otherwise.

> @rwestrel What do you think about the regressions in the scalar cases of this patch?

Shouldn't int `min`/`max` be affected the same way?

I suppose extracting the branch probability from the `MethodData` and attaching it to the `Min`/`Max` nodes is not impossible. I did something like that in the `ScopedValue` PR that you reviewed (and was put on hold). Now, that would be quite a bit of extra complexity for what feels like a corner case.

Another possibility would be to implement `CMove` with branches (https://bugs.openjdk.org/browse/JDK-8340206) or to move the implementation of `MinL`/`MovL` in the ad files and experiment with branches there. 

It seems overall, we likely win more than we loose with this intrinsic, so I would integrate this change as it is and file a bug to keep track of remaining issues.

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

PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2659821025