-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IComparer<T> constraint leads to worse codegen than IComparisonOperators<T,T,bool> constraint #78383
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsWe introduced the new Example: #nullable disable
using System;
using System.Numerics;
using SharpLab.Runtime;
public class C
{
[JitGeneric(typeof(int))]
public static bool M1<T>(T value, T other) where T : IComparable<T>, IComparisonOperators<T, T, bool> =>
value.CompareTo(other) > 0;
[JitGeneric(typeof(int))]
public static bool M2<T>(T value, T other) where T : IComparable<T>, IComparisonOperators<T, T, bool> =>
value > other;
public static bool M3(int value, int other) =>
value.CompareTo(other) > 0;
public static bool M4(int value, int other) =>
value > other;
} Both M1 (generic) and M3 (non-generic) that use L0000: cmp ecx, edx
L0002: jl short L0013
L0004: cmp ecx, edx
L0006: jg short L001a
L0008: xor eax, eax
L000a: test eax, eax
L000c: setg al
L000f: movzx eax, al
L0012: ret
L0013: mov eax, 0xffffffff
L0018: jmp short L000a
L001a: mov eax, 1
L001f: jmp short L000a whereas both M2 (generic) and M4 (non-generic) that use L0000: xor eax, eax
L0002: cmp ecx, edx
L0004: setg al
L0007: ret Related to #78222
|
This still feels somewhat beyond our grasp.For M3, during RBO, we have And we want to recognize that the predicate value returned in BB06 is also a function of the two predicate operands in BB01, so that the entire graph collapses to a single compare. RBO could handle it something like the following:
The prototype changes for #81220 have a parts of this logic, but that analysis won't get triggered by returns, and does not have a sufficiently powerful side-effect analysis (currently tripped up by the assignments to T1). We should be able to argue that the only uses of T1 are in this subgraph (or perhaps, the only uses of the defs of T1 in this subgraph are also in this subgraph) and the only side effects are assignments to T1, so these assignment side effects can be disregarded as we will be removing all the uses too. |
This is going to take more work so it's not going to happen in .NET 8. There is a decent draft PR with some of the necessary bits here: #88527. It doesn't actually find that many cases and introduces a fair amount of new logic, so I'm going to hold off merging this until after .NET 8 is done. |
Still work needed on the approaches taken above -- would be nice if the overall opt was less pattern matchy somehow. For the return cases perhaps postdominance can play a role? Starting at a return (relop), if we walk up the postdominator tree and find a postdominated BBJ_COND, we can do the same sort of symbolic analysis to see if the decision made there determines (or could be modified to determine) the value that will be returned, and if so, perhaps we can short-circuit all the logic in between. For the more general case we may need something like collective postdominance... say two blocks collectively postdominate some nest of computation, we can do similar analysis with the collective postdominator tree, to see if some decision made earlier can be modified to bypass all the nest and arrive at the right partial postdominator. The other missing bit is the ability to recognize when the within-nest side effects' effects are contained to the nest, so if the nest goes away nothing of importance is lost. I'm going to move this out of .NET 9 as it needs more time and thought. |
We introduced the new
IComparisonOperators<,,>
interface as part of the generic math APIs work in .NET 7, but for general-purpose APIs that need to compare and aren't constrained to numerical types, it's preferable from a design perspective to use the longer-standingIComparable<T>
. Unfortunately, the codegen that's produced withIComparable<T>
can end up being worse than that forIComparisonOperators<,,>
, forcing a hard choice. Can we improve the JIT here?Example:
SharpLab
Both M1 (generic) and M3 (non-generic) that use
CompareTo
end up producing:whereas both M2 (generic) and M4 (non-generic) that use
IComparisonOperators<T,T,bool>
end up producing:Related to #78222
cc: @EgorBo, @tannergooding
The text was updated successfully, but these errors were encountered: