Normalize Array.IndexOf result checks to use >= 0

[xtqqczze]

Update checks against the result of Array.IndexOf to consistently use >= 0 instead of comparing directly to -1.

Summary

Typical patterns seen in the codebase:

Array.IndexOf(items, item, 0, _size) != -1;
Array.IndexOf(items, item, 0, _size) > -1;
Array.IndexOf(items, item, 0, _size) == -1;

Normalized forms:

// item exists
Array.IndexOf(items, item, 0, _size) >= 0;

// item does not exist
Array.IndexOf(items, item, 0, _size) < 0;

Rationale

This is a purely mechanical, semantic-preserving change that has some minor codegen benefits.

Diffs: MihuBot/runtime-utils#1526

[tannergooding]

I this feels like a trivial transform that the JIT should be doing instead

The typical pattern is -1 because that is the documented sentinel and I don’t think we want to start deviating from that in managed code as a micro-optimization

Of course not all cases can be pattern matched, but simple comparison transforms or inversions that lead to improved codegen are well within the realm of what the JIT can and should be handling

cc. @EgorBo, @jkotas

[tannergooding]

Just to clarify (based on a conversation on Discord)

I'm not stating the JIT needs to do this optimization. Just rather, if it were beneficial for such an optimization to exist then it would be better for the JIT to provide it.

In general we want to keep our managed code idiomatic and we want the JIT to optimize such idiomatic code so that every user benefits (if enough benefit exists to justify such a transform).

We do not want to be just rewriting existing managed code, especially not to save 2 bytes of codegen for no measurable perf difference or where it deviates from the existing "standard" pattern that most users write.

[jkotas]

I this feels like a trivial transform that the JIT should be doing instead

I am not sure what kind of bitwise tricks can the JIT do compared to the ones that it does today. I do not think that the JIT can do this well in all cases. Number of architectures have special instructions for branching when register value is negative, but there are no such instructions for branching when register value is -1.

Personally, I like comparing with 0 better style-wise. Some methods (like IndexOf) are guaranteed to return -1 sentinel, some methods are just guaranteed to return negative value (like Compare). Comparing with 0 works in all cases. Slightly better codegen for comparing with zero is nice side-effect.

[xtqqczze]

I this feels like a trivial transform that the JIT should be doing instead

The JIT can already transform patterns like > -1, but other patterns like != -1 cannot be transformed without the JIT having some intrinsic recognition of methods like IndexOf that never return a value less than -1.

[jkotas]

The typical pattern is -1 because that is the documented sentinel

I have not done exact counts, but both comparing IndexOf result with -1 and comparing with 0 seems to be very popular.

[stephentoub]

Personally, I like comparing with 0 better style-wise.

+1

[tannergooding]

I'm fine with changing the convention if we all agree that >= 0 is better.

It might be good (as a bonus) to document that as part of our coding-style conventions then and potentially to have an analyzer for such cases.

[jkotas]

FWIW, comparing result of Array.IndexOf with 0 seems to be the more common pattern under libraries in this repo:

8x Array.IndexOf.*\< 0
20x Array.IndexOf.*\>= 0

9x Array.IndexOf.*== -1
7x Array.IndexOf.*!= -1

[jkotas]

/ba-g timeout, intermittent build crash without details