@dotnet-policy-service agree

@kg, PTAL.

This LGTM but will need to pass CI before it can be merged. Let me know if you need help figuring out why the runtime tests lanes are red.

@Lotendan to elaborate on the above, technically, this code:

popcnt    edi, edi
jg       SHORT G_M000_IG04

is an invalid replacement for

popcnt   edi, edi
test     edi, edi
jg       SHORT G_M000_IG04

because the latter sets SF explicitly before checking it with jg, while the former clears the flag unconditionally and still checks its value.

In the case of popcnt, the result can never be negative, so the value of SF would be zero regardless of whether it was unconditionally cleared or set according to the result value. That means the invalid optimization won't actually break in this case, but it could with other instructions that get caught by the same logic.

If you instead introduce an assertion (see assertionprop.cpp) that the value is never negative, JIT can transform the compare from > 0 to != 0, which results in this valid code:

popcnt   edi, edi
jne      SHORT G_M000_IG04

Ex: https://godbolt.org/z/Mzn4Kbdas

Thank you for the hint about the assertions. Very helpful as I'm totally new to this code base.
As far as I understand your suggestion it means we need two steps:

1. skip emitting a test instruction. If not done with flags, is that acceptable to remain conservative and "hardcode" the instruction? e.g. if POPCNT.
2. use assertions to maybe modify the GenTree and transform a >0 into a !=0 in case we know the result is always positive. I have browsed assertionprop.cpp quickly and intend to use optAssertionProp. Any hint about where this code best belongs?

If you add the assertion, you don't have to worry about removing the test -- that will happen automagically, because JIT will see that the value can't be negative and that > 0 is the same as the simpler != 0. And since popcnt sets ZF, JIT already knows the flags are correctly set for jne.

This is actually already handled for explicit calls to Popcnt.PopCount as shown here: https://godbolt.org/z/3GEhaen5E

I believe all you need to do is duplicate this range assertion:

for NI_PRIMITIVE_PopCount (and LeadingZeroCount + TrailingZeroCount)

@Lotendan I gave this a closer look today, and I have a couple of corrections

1. Apparently, I hallucinated the optimization that would convert > 0 to != 0 with a range assertion. The reason Popcnt.PopCount emits optimal code today is that it has an unsigned return type. [unsigned] > 0 is optimized to != 0 here:

2. The range assertion I pointed out above already applies to NI_PRIMITIVE_PopCount because it is converted to NI_X86Base_PopCount (on xarch) on import. You can see that in action with:

static void M2(int value)
{
    if (int.PopCount(value) < 0) 
        throw null!;
}

which already compiles to just

C:M2(int) (FullOpts):
       ret  

because JIT is already seeing that the value can't be negative.

So, if you were to add the missing > 0 to != 0 optimization for known non-negative signed values (it could be added in morph as above), that should result in more globally useful improvements.

Your intuition was good and gave me the right tools to progress on this bugfix.
At this code location, for a signed integer the expression cmp->IsUnsigned() evaluates to false.
However, if the compare is a signed compare and the operand is known to be never negative, as is the case with POPCNT, then we can also trigger the optimization.
With this done I have compiled these 3 functions below:

public static void M1(int value)
{
    if (int.PopCount(value) > 0) 
        throw null!;
}
public static void M2(uint value)
{
    if (uint.PopCount(value) > 0)
        throw null!;
}
public static void M3(int value)
{
    if (Popcnt.PopCount((uint) value) > 0) 
        throw null!;
}

And they all end up as:

IN0001: 000003 popcnt   ecx, ecx
IN0002: 000007 jne      SHORT G_M1455_IG04

Replacing with LeadingZeroCount everywhere leads to:

IN0001: 000003 lzcnt    ecx, ecx
IN0002: 000007 jne      SHORT G_M1455_IG04

Same for TrailingZeroCount

IN0001: 000003 tzcnt    ecx, ecx
IN0002: 000007 jne      SHORT G_M28251_IG04

Nice! I suspected this might catch Span.Length > 0 as well, and the diffs show a lot of hits for that on Arm64. ex:

-            cmp     w1, #0
-            bgt     G_M13911_IG05
+            cbnz    w1, G_M13911_IG05

@kg sorry to step on your toes on this one. Can you give it another look?

The new changes LGTM. Thanks for providing guidance @saucecontrol , I appreciate having someone who knows the JIT well chime in.

@kg what are the next steps then to merge this PR? :)

Now that checks have passed I'm going to get the OK to merge it since I'm new to the JIT. I'm not sure how long that will take, thanks for your patience!

@dotnet/jit-contrib PTAL

Thanks, I have applied the suggestions.

@Lotendan can you update this branch and re-run CI?

Rebased this branch on the main branch :)

@Lotendan I think we are good to merge this! You can do the honors if you'd like! Thank you for this contribution!

Thanks @adamperlin ! I don’t have merge permissions in this repo, so feel free to merge it when convenient ;)