Move GenTreeIntCon::gtCompileTimeHandle field to be a global map

[EgorBo]

This PR removes GenTreeIntCon::gtCompileTimeHandle field in favor of a global hashmap (we already had it in VN).

• This allows to remove a TODO where we used to give on 0 compile time handles in VN
• Fixes a bug @kunalspathak found

Fixes: #114823

[dotnet-policy-service]

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

[jakobbotsch]

we already had it in VN

Having internal maps in phases that keep them maintained is not as problematic as having global ambient state throughout the JIT that must be maintained by all phases and becomes part of the IR invariants.

Though, given that this is a map from the value itself and not from GenTree*, it probably does not come with the same downsides, so perhaps it's not too bad.

[SingleAccretion]

An alternative to this would be to introduce a JIT-EE API for embedded handle -> compile time handle.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[EgorBo]

Ok I ended up reverting my attempts to either use a global hash or JIT-EE - they both were not trivial, and just removed unused JIT-EE calls and add "!=0" checks to the VN's compilteTimeHandle cache. We already had that !=0 check here

runtime/src/coreclr/jit/valuenum.cpp

Lines 4696 to 4706 in dcf79a4

	const bool found0 = m_embeddedToCompileTimeHandleMap.TryGetValue(handleVal0, &compileTimeHandle0);
	const bool found1 = m_embeddedToCompileTimeHandleMap.TryGetValue(handleVal1, &compileTimeHandle1);
	assert(found0 && found1);
	// We may see null compile time handles for some constructed class handle cases.
	// We should fix the construction if possible. But just skip those cases for now.
	//
	if ((compileTimeHandle0 == 0) || (compileTimeHandle1 == 0))
	{
	return NoVN;
	}

I just added it to other uses of that map.

PTAL @jakobbotsch @kunalspathak @dotnet/jit-contrib

[jkotas]

Isn't this introducing an abstraction violation? CORINFO_XXX_HANDLEs are not meant to be embedded in the code. We have an intentional distinction between the CORINFO_XXX_HANDLE handles used to represent type system entities on JIT/EE interface and how these values get embedded into the code.

It happens to work since these module handles are only used with JIT currently, but this change does not look like an improvement to me.

[EgorBo]

@jkotas I just assumed that almost 300 LOC is too much for an API that is just no-op (and CoreCLR only), I can revert this part.

Looks like the only case where we need to embed a field handle is CoreCLR jitting a method that accesses a field it has no access for, so JIT emits an exception telling about the field access violation. Isn't part of the IL verification that we got rid some time ago?

As for the modules, I presume they're only used to embed strings as helper calls, so JIT only.

[jkotas]

300 LOC

It is more like 20 lines of actual code. The rest is boiler plate.

I can revert this part.

I think the part that's introducing JIT/EE interface abstraction violations should be reverted.

Looks like the only case where we need to embed a field handle is CoreCLR jitting a method that accesses a field it has no access for, so JIT emits an exception telling about the field access violation. Isn't part of the IL verification that we got rid some time ago?

We have not removed visibility checks to enforce good hygiene.

CORINFO_HELPER_DESC/impInsertHelperCall is meant to be a general-purpose facility to replace a method call or field access with a call to a helper that throws an exception. Doing it this way allows debugger to stop at the line that failed the visibility check.

If we were to fail the JITing of the whole method instead, the user would just see that there is a visibility problem somewhere in method, without seeing the precise location.

I presume they're only used to embed strings as helper calls, so JIT only.

There is a TODO to implement this optimization for R2R

runtime/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

Lines 3591 to 3592 in 8df4955

	// TODO: Lazy string literal helper
	return CorInfoHelpFunc.CORINFO_HELP_UNDEF;

.

Where possible, we should be building JIT/EE interactions such that they are JIT/AOT agnostic even if it is not needed at the moment.

[jkotas]

Same here