On Thu, 13 Feb 2025 16:43:22 GMT, Roland Westrelin <rol...@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
