why not just rangeType = TYP_UINT like above?

Because LowerBoundForType doesn't handle TYP_UINT, only UpperBoundForType does, and so ForType would hit an unreached.

I believe this is intentional and to avoid bugs since we shouldn't normally encounter TYP_UINT for anything except rare special scenarios like this

I really don't like the fact we need to pass type. It should be evaluated from VN, shouldn't it?

The issue is the initial if ((num == ValueNumStore::NoVN) || (budget <= 0)) check in GetRangeFromAssertions.

i.e, handling producing a range if no VN exists, which previously relied on the fact we would only ever have TYP_INT, but now we can have TYP_LONG as well.

We'd have to have no VN produce keUnknown and for the callers to assume a constant range based on the type in that case instead, which is additional churn either way. I don't have a strong preference here on either approach, and went with passing the type through to avoid regressing the status quo.

so can we return just keUnknown if no VN? since this function already may return keUnknown

We could, but then that will regress TYP_INT scenarios with no VN where we previously would've gotten an appropriate [INT32_MIN, INT32_MAX] constant range.

That might be fine, since most things should have VNs, but it might also not be since we lose VN info in various places or may not have it for new nodes.

I could restrict it to just GetRangeFromAssertions, have it take the tree, extract the VN, and then call GetRangeFromAssertionsWorker which doesn't further propagate the type since it must be from a VN at that point, but I think that's the "best" we can do since we need a range for two possible types now.

it's unfortunate we broke the contract for GetRangeFromAssertions to always return a constant range. My opinion we shouldn't do it and instead either upgrade Range to TYP_LONG or introduce a new GetRangeFromAssertions for 64-bit ranges

or introduce a new GetRangeFromAssertions for 64-bit ranges

This is a lot of unnecessary code duplication and keeping the same overall handling regardless. That is, it doesn't change what assertionprop has to handle here, rather it just forces it to two paths one calling GetRangeFromAssertions32 and one calling GetRangeFromAssertions64 and still handling the fact that one path may not produce a constant range. It saves nothing and just makes things more complex.

With this approach, we have a single method handling both and the caller just has to handle the fact it might not be constant, rather than having to own dispatch to the right method and still handle that nuance anyways.

instead either upgrade Range to TYP_LONG

This is the eventual goal, but I'm trying to get this done incrementally and in a way that is easier to test, review, and handle.

Fully handling TYP_LONG is quite a bit more complex than only extending it to handle places where FitsIn<int32_t> remains "trivially true".

Namely, it involves extending Range/Limit to track int64_t cns, to then have all RangeOperations support int64_t, to have additional range ops that handle the 32 vs 64-bit limits, to ensure we understand ADD(int, x, y) and ADD(long, x, y) have different overflow limits to check and track for example, and to have the various handlers consider those nuances as well.

I still don't understand what 0, keUnknown is supposed to mean, keUnknown implies it can be something that overflows making the range invalid

The point here is more conceptual and using it as a sentinel because we don't just use it for overflow, but just generally as a "limit cannot be determined/represented". It could well just be something like say 0, keMaxValue instead.

The general point, however, is that while Range is limited to int32_t, it may still be beneficial to propagate up that we know the lower bound and simply cannot represent the upper bound, thus a given TYP_LONG is known to be "never negative" and all the usual "known unsigned value" optimizations can kick in.

Well, this problem won't exist if we we make Range 64bit (or Range)

Right, which is an eventual goal. I'm going to try to do it after this PR lands even, its just a much more involved change and has potentially reaching arms into many other places in the JIT where we're limiting checks to just genActualType() == TYP_INT

I suspect it's fine to always give up on gc types unconditionally

We are always giving up unconditionally? The difference is just preserving the status quo where it returned a constant range for GC types on 32-bit.

I don't think it's useful, just some weird scenario where a TYP_INT PHI had a byref PHI_ARG on 32bit, I doubt we can ever deduce anything from that anyway, and it's 32bit

Right, but the same consideration will exist when we eventually extend this to full 64-bit. So we basically want to make a decision of "return the full range in that scenario, if we can" or "always return keUnknown". I deferred to maintaing the status quo since that's the less risky change.