JIT: Generate `v{min/max}ss` for `a > b ? a : b` pattern instead of cmp + jmp

[BoyBaykiller]

https://godbolt.org/z/1W83cYox1

float Max(float a, float b)
{
 return a > b ? a : b;
}

Program:Max(float,float):float:this (FullOpts):
- vucomiss xmm1, xmm2
- jbe SHORT G_M57467_IG05
- vmovaps xmm0, xmm1
- ret 
-G_M57467_IG05: ;; offset=0x000B
- vmovaps xmm0, xmm2
+ vmaxss xmm0, xmm1, xmm2
 ret 

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[tannergooding]

I don't think this one is worth implementing in the JIT.

It'd be easier for someone to add a general code fixer to change this pattern to float.MaxNative instead

[tannergooding]

-- We have an approved analyzer for suggesting such modern code replacements for numerics and hardware intrinsics in general. The work just needs to be done here.

[BoyBaykiller]

There are already optimizations in place for similar patterns that are less common than min/max, for example: #66193

An analyzer is not as robust in detecting this pattern as having it in the JIT. What I mean is that it might occur without the user explicitly writting it out. If implemented in JIT languages other than C# can benefit from it without needing an explicit API + analyzer support.

I don't think this one is worth implementing in the JIT.
It'd be easier for someone to add a general code fixer to change this pattern to float.MaxNative instead

Then writing these transformations in the JIT should become simpler. I've seen this mentioned by @EgorBo a couple times: #66561 (comment)

I understand that implementing a transformation like this in the current-state-JIT might not be worth it, but I don't like moving the problem to an analzyer as a solution

[tannergooding]

There’s a big difference between something like blsi, which is a trivial pattern that doesn’t have a corresponding general purpose method; and something like max, which is a more complex pattern (due to the branch) that has a dedicated API (and which has had some kind of dedicated api for a long time)

blsi is also notably not that uncommon, it semi-regularly occurs in interop and code dealing with bit fields

The max pattern however also has additional complexity and nuance to it, due to IEEE 754 semantics and due to how users may end up writing it. It is really something that is best suited to an analyzer, as is the case for the plethora of other code fixer improvements we have for much more perf critical and common scenarios

[BoyBaykiller]

There’s a big difference between something like blsi, which is a trivial pattern that doesn’t have a corresponding general purpose method; and something like max, which is a more complex pattern

The patterns are still equivalent in that both have explicit CPU/instruction support and it is the job of an optimizing compiler to use that (the big 3 C++ compilers all do) and not defer the problem to an analyzer. And there is Bmi1.ExtractLowestSetBit but I guess that doesn't count as general purpose.

The max pattern however also has additional complexity and nuance to it, due to IEEE 754 semantics and due to how users may end up writing it

Do you mean passing the arg in the correct order (so that NaN behaviour is the same)? Doesn't seem like a big issue.

blsi is also notably not that uncommon, it semi-regularly occurs in interop and code dealing with bit fields

So that's a use-scenario that was not thought of when the blsi PR landed. The same can be said about other transformations. You never know where it ends up...

---

And correct me if I am wrong but a > b ? a : b and float.MaxNative are not even the same.
The first says: Do exactly this and if possible optimize using intrinsic (assuming this transformation were to be implemented)
The later says: Use whatever native max the machine offers allowing platform specific behaviour (arround NaN and such)

[tannergooding]

it is the job of an optimizing compiler to use that (the big 3 C++ compilers all do)

The C++ compilers all have a number of optimizations that RyuJIT and many other modern compilers will never get. In part because such optimizations are unnecessary and slow down the overall compilation without providing meaningful benefit to real world apps.

not defer the problem to an analyzer.

Modern compilers are explicitly looking to target analyzers instead. They can be run dynamically in the background for lower cost and encourage code to be normalized to more correct patterns. The analyzer has direct access to the original syntax and so can more easily handle complex patterns, code flow, and other scenarios. It also has benefit in that it encourages codebases to improve over time, can be updated "out of band" from the runtime, etc.

And there is Bmi1.ExtractLowestSetBit but I guess that doesn't count as general purpose.

No, it is a hardware specific intrinsic instead. Several architectes have a similar instruction and standard libraries rarely provide such a function, so it is typical for the compiler to do the trivial recognition and expansion.

So that's a use-scenario that was not thought of when the blsi PR landed. The same can be said about other transformations. You never know where it ends up...

It was explicitly known, even if it wasn't called out. It is often known where expansions will turn up and where it is beneficial to provide them. The biggest unknown tends to be across inlining boundaries, but the naive min/max patterns aren't expected to show up in such cases.

Do you mean passing the arg in the correct order (so that NaN behaviour is the same)? Doesn't seem like a big issue.

There's a lot of subtle nuance and little differences that break the optimization. Things that a user cannot know without an analyzer.

The intent of all such patterns is typically to just emit a "fast max" check, however; and it is reasonable for developers to be suggested they replace it with an appropriate standard API call, which has other benefit of correctly handling constant folding, value numbering, and other simplifications.

And correct me if I am wrong but a > b ? a : b and float.MaxNative are not even the same.

Correct, which is part of the complexity and why doing this in the JIT is more problematic; because it won't always be valid or desirable.

It is explicitly better to encourage devs to switch over to one of the standard Max APIs instead; and if they really care about the cycle or two difference caused by the branch then they have other options available.

The cost of adding the optimization to the JIT, supporting it over time, educating users on the considerations, impact to JIT throughput, and other things just make it not a good choice in comparison to the alternatives. It's much better suited to be an analyzer and to invest the JIT effort towards more meaningful optimizations.

[BoyBaykiller]

Ok I'll leave it at that. Thanks for your time!