Elide redundant test instruction for signed comparison predicates

[ashay]

Prior to this patch, when emitting code for x64, if an instruction
updated the flags register and if the following instruction was an
equals or not-equals comparison with zero, the runtime elided the test
instruction for materializing the comparison.

Although correct, this is limiting, since it excludes the predicates
SGT, SGE, SLT, and SLE. This patch extends the optimization to allow the
omission of the test instruction for all signed integer comparisons.
We skip unsigned integer comparisons since unsigned comparisons with
zero can be evaluated earlier in the compilation process.

[ashay]

@jakobbotsch @xtqqczze Sorry about the noise, but here's the updated PR after incorporating xtqqczze's changes.

[dotnet-policy-service]

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

[MichalPetryka]

No diffs?

[ashay]

No diffs?

Huh.. I guess we never hit the case in any of the tests. I'm not sure what the project policy is, but if this patch isn't useful, I'm happy to close the PR.

[xtqqczze]

No diffs?

No diffs appear because the current change has a redundant check for whether the carry flag is reset. This does not address #117866, since instructions such as add write the carry flag.

Reference: instrsxarch.h#L76

[ashay]

No diffs appear because the current change has a redundant check for whether the carry flag is reset.

But add doesn't reset OF either (it writes to it). So if the check were to be reduced to DoesResetOverflowFlag(), it still wouldn't fire for the add instruction, would it?

[xtqqczze]

No diffs appear because the current change has a redundant check for whether the carry flag is reset.

But add doesn't reset OF either (it writes to it). So if the check were to be reduced to DoesResetOverflowFlag(), it still wouldn't fire for the add instruction, would it?

Ah yes, this would only match:

• blsi | Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Writes_CF
• blsmsk | Resets_OF | Writes_SF | Resets_ZF | Undefined_AF | Undefined_PF | Writes_CF
• blsr | Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Writes_CF
• bzhi | Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Writes_CF

[xtqqczze]

We can still optimise some cases, like the following, at least:

void M(int x)
{
    if ((x & (x - 1)) <= 0) // blsi
        throw null!;
}

sharplab

[JulieLeeMSFT]

@tannergooding, please review this PR.

[xtqqczze]

@ashay Pipeline build has been deleted, please update with merge commit to restart CI.

[Copilot]

Pull Request Overview

This PR extends an x64 code generation optimization to eliminate redundant test instructions for signed comparison predicates. Previously, the optimization only applied to equals/not-equals comparisons with zero, but now includes all signed integer comparisons (SGT, SGE, SLT, SLE).

• Adds support for omitting test instructions when using signed comparison predicates
• Introduces a new helper function to check if instructions reset the overflow flag
• Extends the existing AreFlagsSetToZeroCmp function with additional condition checks

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/coreclr/jit/emitxarch.h	Adds declaration for new DoesResetOverflowFlag helper function
src/coreclr/jit/emitxarch.cpp	Implements DoesResetOverflowFlag and extends AreFlagsSetToZeroCmp with signed comparison optimizations

[ashay]

Thanks @xtqqczze, I merged the main branch just now.

[xtqqczze]

Diffs are not an improvement.

[xtqqczze]

This PR does not seem fruitful; the issue (#117866) it was intended to address was closed as invalid.

[JulieLeeMSFT]

@ashay, please address the feedback.

This PR does not seem fruitful; the issue (#117866) it was intended to address was closed as invalid.

[ashay]

Closing this PR since the issue associated with this change was marked as invalid.