Skip to content

Conversation

@AndyAyersMS
Copy link
Member

We increasingly see constant byrefs. Make sure those don't trip up range-based optimizations.

Fixes #116861.

We increasingly see constant byrefs. Make sure those don't trip up range-based optimizations.

Fixes dotnet#116861.
Copilot AI review requested due to automatic review settings July 1, 2025 21:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds an early exit in the relational comparison optimizer to skip range-based optimizations when either operand isn’t an integral type.

  • Caches op1 and op2 for reuse
  • Checks both operand types with varTypeIsIntegral
  • Returns immediately if a non-integral type is detected
Comments suppressed due to low confidence (1)

src/coreclr/jit/morph.cpp:9071

  • Consider adding unit tests to cover the new early-return path for non-integral operands (e.g., floating-point constants) to verify that the optimizer skips range logic as intended.
    if (!varTypeIsIntegral(op1->TypeGet()) || !varTypeIsIntegral(op2->TypeGet()))

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 1, 2025
@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

@dotnet-policy-service
Copy link
Contributor

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

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr libraries-pgo

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nuance between integer and byref constants seems to come up quite a bit.

I wonder if we'd have less overall problems if we had CNS_INT and CNS_REF here, so we could be more explicit about the two concepts and what is safe for each (or something like IsIntegralConst(/*allowByref*/ true) where it defaults to not including ref/byref typed integer constants.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jul 2, 2025

libraries-pgo failure is unrelated:

    Microsoft.Extensions.Hosting.Internal.HostTests.HostStopAsyncCanBeCancelledEarly [FAIL]
      Assert.Equal() Failure: Values differ
      Expected: Task<VoidTaskResult,<StopAsync>d__16> { Status = RanToCompletion }
      Actual:   Task { Status = RanToCompletion }

opened #117225

@AndyAyersMS AndyAyersMS merged commit b509ab1 into dotnet:main Jul 2, 2025
123 of 134 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JIT: Assertion failed 'varTypeIsIntegral(node)'

3 participants