Skip to content

Have RangeCheck::ComputeRange handle some TYP_LONG scenarios #128676

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.

Sign up for GitHub

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

Closed
tannergooding wants to merge 6 commits into from
Closed

Have RangeCheck::ComputeRange handle some TYP_LONG scenarios #128676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b105749
Have RangeCheck::ComputeRange handle some TYP_LONG scenarios
tannergooding May 28, 2026
d59545a
Apply formatting patch
tannergooding May 28, 2026
2d548e6
Handle the cast fromType not being TYP_UINT and allowing a shift amou…
tannergooding May 28, 2026
ff25dae
Improve wording on two comments
tannergooding May 28, 2026
968d423
use `IsVNIntegralConstant`
tannergooding May 29, 2026
3a9508d
Fix a typo, comment, and build failure
tannergooding May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 172 additions & 10 deletions src/coreclr/jit/rangecheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1759,31 +1759,100 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool
switch (binop->OperGet())
{
case GT_ADD:
{
r = RangeOps::Add(op1Range, op2Range);
break;
}

case GT_MUL:
{
r = RangeOps::Multiply(op1Range, op2Range);
break;
}

case GT_RSH:
{
if (varTypeIsLong(binop))
{
int shiftAmount;

if (op2Range.IsSingleValueConstant(&shiftAmount) && (shiftAmount >= 32) && (shiftAmount < 64))
{
// The upper 33-bits will all match post shift, so we are within [INT32_MIN, INT32_MAX]
// and can further reduce based on the remaining shift amount.

op1Range = GetRangeFromType(TYP_INT);
op2Range = Range(Limit(Limit::keConstant, shiftAmount - 32));
}
else
{
return Range(Limit::keUnknown);
}
}

r = RangeOps::ShiftRight(op1Range, op2Range, /*logical*/ false);
break;
}

case GT_RSZ:
{
if (varTypeIsLong(binop))
{
int shiftAmount;

if (op2Range.IsSingleValueConstant(&shiftAmount) && (shiftAmount >= 33) && (shiftAmount < 64))
{
// The upper 33-bits must all be zero post shift, so we are within [0, INT32_MAX]
// and can further reduce based on the remaining shift amount. This is notably one
// higher than RSH since we'd otherwise get a value within [INT32_MAX + 1, UINT32_MAX]

op1Range = Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, INT32_MAX));
op2Range = Range(Limit(Limit::keConstant, shiftAmount - 33));
}
else
{
return Range(Limit::keUnknown);
}
}

r = RangeOps::ShiftRight(op1Range, op2Range, /*logical*/ true);
break;
}

case GT_LSH:
{
if (varTypeIsLong(binop))
{
// We can't handle LSH for long since we don't know the state of the upper 32-bits
return Range(Limit::keUnknown);
}

r = RangeOps::ShiftLeft(op1Range, op2Range);
break;
}

case GT_AND:
{
r = RangeOps::And(op1Range, op2Range);
break;
}

case GT_OR:
{
r = RangeOps::Or(op1Range, op2Range);
break;
}

case GT_UMOD:
{
r = RangeOps::UnsignedMod(op1Range, op2Range);
break;
}

default:
{
return Range(Limit::keUnknown);
}
}

JITDUMP("BinOp %s %s %s = %s\n", op1Range.ToString(m_compiler), GenTree::OpName(binop->OperGet()),
Expand Down Expand Up @@ -2189,18 +2258,20 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreas
range = Range(Limit(Limit::keUnknown));
JITDUMP("GetRangeWorker not tractable within max stack depth.\n");
}
// TYP_LONG is not supported anyway.
else if (expr->TypeIs(TYP_LONG))
{
range = Range(Limit(Limit::keUnknown));
JITDUMP("GetRangeWorker long, setting to unknown value.\n");
}
// If VN is constant return range as constant.
else if (m_compiler->vnStore->IsVNConstant(vn))
{
range = (m_compiler->vnStore->TypeOfVN(vn) == TYP_INT)
? Range(Limit(Limit::keConstant, m_compiler->vnStore->ConstantValue<int>(vn)))
: Limit(Limit::keUnknown);
int32_t cns;
if (m_compiler->vnStore->IsVNIntegralConstant(vn, &cns))
{
Comment thread
tannergooding marked this conversation as resolved.
range = Range(Limit(Limit::keConstant, static_cast<int32_t>(cns)));
}
else
{
// TODO: We may want to also check for `uint32_t` since that allows knowing a TYP_LONG is never negative
range = Limit(Limit::keUnknown);
JITDUMP("GetRangeWorker unsupported VN constant, setting to unknown value.\n");
}
}
// If local, find the definition from the def map and evaluate the range for rhs.
else if (expr->IsLocal())
Expand Down Expand Up @@ -2253,8 +2324,39 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreas
}
else if (expr->OperIs(GT_CAST))
{
GenTreeCast* cast = expr->AsCast();

var_types toType = cast->CastToType();
var_types fromType = cast->CastFromType();

if (cast->IsUnsigned())
{
fromType = varTypeToUnsigned(fromType);
}

var_types rangeType;

if (genTypeSize(fromType) < genTypeSize(toType))

@EgorBo EgorBo May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I ran diffs with this change and there were 0 diffs. can we remove it? I really don't see what value it brings to a very buggy CompureRange in rangecheck that typically requires all changes to be repeated in DoesOverflow as well.

So it's just + 30 LOC with no value that makes me uncofortable knowing how fragile ComputeRange is (it's a completely separate thing vs GetRangeFromAssertions/VN)

@tannergooding tannergooding May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can remove it, but could you elaborate on what is uncomfortable about the check?

We presumably want the same type of check in GetRangeFromAssertions which is also using the casts target type as the initial foundation.

A widening cast can never exceed the bounds of the input (fromType), so:

  • BYTE -> INT: [INT8_MIN, INT8_MAX]
  • UBYTE -> INT: [0, UINT8_MAX]
  • SHORT -> INT: [INT16_MIN, INT16_MAX]
  • USHORT -> INT: [0, UINT16_MAX]
  • INT -> LONG: [INT32_MIN, INT32_MAX]
  • UINT -> LONG: [0, UINT32_MAX]

A cast between same sizes however only respects the target type (and even should've been elided).

  • UINT -> INT: [INT32_MIN, INT32_MAX]
  • ULONG -> LONG: [INT64_MIN, INT64_MAX] (i.e. keUnknown)

We then cannot have casts between the same type where the target is unsigned, because we always treat the destination as signed. And casts between same sized small types likewise simply become widening to INT.

A narrowing cast then is restricted to the bounds of the output (toType), so:

  • LONG -> INT: [INT32_MIN, INT32_MAX]
  • ULONG -> INT: [INT32_MIN, INT32_MAX]
  • INT -> BYTE: [INT8_MIN, INT8_MAX]
  • INT -> UBYTE: [0, UINT8_MAX]
  • INT -> SHORT: [INT16_MIN, INT16_MAX]
  • INT -> USHORT: [0, UINT16_MAX]

@EgorBo EgorBo May 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

UINT -> LONG: [0, UINT32_MAX]

Range doesn't support unsigned upper bound so if your change does it - it's already a bug. Given no diffs it means it will fail in someones Production and not in out suite.

I think if code brings no obvious value it should be removed. In fact, I'd like us to avoid doing any changes to ComputeRange/DoesOverflow. This code particually looks like it should just call GetRangeFromAssertions for casts.

@EgorBo EgorBo May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That being said, bugs in branch optimizations are the worst - they always lead to silent codegen bug (as opposite to deterministic asserts/crashes) extremely difficult to investigate. I've spent enought time on them in these things to only accept changes that have clear impact/coverage

@tannergooding tannergooding May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Range doesn't support unsigned upper bound so if your change does it - it's already a bug

It does not do that, I was simply giving the full list. We use GetRangeForType and so TYP_INT becomes [INT32_MIN, INT32_MAX] and TYP_UINT, TYP_LONG, and TYP_ULONG all become keUnknown

@EgorBo EgorBo May 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Anyway, if you really want to make this change, just call GetRangeFromAssertion on CAST's VN, it's literally one line change vs 30 LOC of untested code

@tannergooding tannergooding May 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That being said, bugs in branch optimizations are the worst - they always lead to silent codegen bug (as opposite to deterministic asserts/crashes) extremely difficult to investigate. I've spent enought time on them in these things to only accept changes that have clear impact/coverage

Agreed, but lack of diffs is not representative of lack of coverage and I would argue in many cases such problems occur because we tried to be clever and not simply do the more obviously correct handling. -- i.e. I actually think the current logic is much more likely to be broken than the logic in this PR, because it is trying to be simple and only handle the case it thinks it cares about. While the new logic explicitly factors in widening vs narrowing vs same size with concrete reasoning as to why it is correct by definition.

I'm Still fine to remove this part if its actually giving zero diffs (will wait until after the IsVNIntegralConst fix goes in though), but I don't agree with the doing so because of the shape of the logic itself.

{
// We're going from a small type to a large type
// and so regardless of whether we zero or sign-extend
// the value is preserved within the confines of its
// original input for the destination, i.e. it always
// passes the FitsIn<fromType> check.

rangeType = fromType;
Comment thread
tannergooding marked this conversation as resolved.
}
else
{
// We're either going from a big type to a small type
// or between signed and unsigned types of the same size
// so we want to use toType as the range.

rangeType = toType;
}

// TODO: consider computing range for CastOp and intersect it with this.
range = GetRangeFromType(expr->AsCast()->CastToType());
range = GetRangeFromType(rangeType);
}
else if (expr->OperIs(GT_ARR_LENGTH))
{
Expand All @@ -2271,6 +2373,66 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreas
range = Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, CORINFO_Array_MaxLength));
}
}
else if (expr->OperIs(GT_INTRINSIC))
{
GenTreeIntrinsic* intrinsic = expr->AsIntrinsic();
NamedIntrinsic ni = intrinsic->gtIntrinsicName;

switch (ni)
{
case NI_PRIMITIVE_LeadingZeroCount:
case NI_PRIMITIVE_TrailingZeroCount:
case NI_PRIMITIVE_PopCount:
{
var_types op1Type = intrinsic->gtGetOp1()->TypeGet();

range.lLimit = Limit(Limit::keConstant, 0);
range.uLimit = Limit(Limit::keConstant, varTypeIsLong(op1Type) ? 64 : 32);
break;
}

default:
{
range = Range(Limit(Limit::keUnknown));
break;
}
}
}
#if defined(FEATURE_HW_INTRINSICS)
else if (expr->OperIs(GT_HWINTRINSIC))
{
GenTreeHWIntrinsic* hwintrinsic = expr->AsHWIntrinsic();
NamedIntrinsic ni = hwintrinsic->GetHWIntrinsicId();

switch (ni)
{
#if defined(TARGET_XARCH)
case NI_AVX2_LeadingZeroCount:
case NI_AVX2_TrailingZeroCount:
case NI_AVX2_X64_LeadingZeroCount:
case NI_AVX2_X64_TrailingZeroCount:
case NI_X86Base_PopCount:
case NI_X86Base_X64_PopCount:
#elif defined(TARGET_ARM64)
case NI_ArmBase_LeadingZeroCount:
case NI_ArmBase_Arm64_LeadingZeroCount:
#endif
{
var_types op1Type = hwintrinsic->Op(1)->TypeGet();

range.lLimit = Limit(Limit::keConstant, 0);
range.uLimit = Limit(Limit::keConstant, varTypeIsLong(op1Type) ? 64 : 32);

@EgorBo EgorBo May 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the reason why the original path was so simple because only "never negative" knowledge had an impact. Upper bound didn't matter.

@tannergooding tannergooding May 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but there's also no reason to not be accurate and it can have impact in user code. Namely this allows indexing into a span/array with known length to skip bounds check and therefore allowing such code to avoid unsafe.

break;
}

default:
{
range = Range(Limit(Limit::keUnknown));
break;
}
}
}
#endif
else
{
// The expression is not recognized, so the result is unknown.
Expand Down
16 changes: 16 additions & 0 deletions src/coreclr/jit/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,22 @@ class BitOperations

static uint64_t RotateRight(uint64_t value, uint32_t offset);

static uint32_t RoundUpToPowerOf2(uint32_t value)
{
return static_cast<uint32_t>(0x1'0000'0000ULL >> LeadingZeroCount(value - 1));
}

static uint64_t RoundUpToPowerOf2(uint64_t value)

@EgorBo EgorBo May 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is basically a dead code

@tannergooding tannergooding May 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, the PR was originally doing more and I dropped some of the changes to make it more iterative and easier to review and check for correctness.

I can drop it and add it back in the PR that actually uses it instead, but we do have a few such "round up" in the codebase already and so if we keep it, we can just submit a follow up PR to centralize them to this one instead (as we've done with log2 and other helpers that are in utils/BitOperations

{
if ((value == 0) || (value > 0x8000'0000'0000'0000ULL))
{
return 0;
}

uint64_t shift = 64 - LeadingZeroCount(value - 1);
return (1ULL ^ (shift >> 6)) << shift;
Comment thread
tannergooding marked this conversation as resolved.
Comment thread
tannergooding marked this conversation as resolved.
}

static uint32_t SingleToUInt32Bits(float value);

static uint32_t TrailingZeroCount(uint32_t value);
Expand Down
Loading