Avoid handing out duplicate CORINFO_MODULE_STRUCT_* handles

[MichalStrehovsky]

RyuJIT depends on never seeing two different CORINFO_MODULE_STRUCT for the same thing.

Fixes #93843.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

RyuJIT depends on never seeing two different CORINFO_MODULE_STRUCT for the same thing.

Fixes #93843.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[vitek-karas]

Just for my education - this is a per-method cache, as in, we're going to clear it after each method compilation. And JIT doesn't maintain any state between methods either, right?

[MichalStrehovsky]

Yes, it's per method and there's not supposed to be extra state on the JIT side either.

[jkotas]

RyuJIT depends on never seeing two different CORINFO_MODULE_STRUCT for the same thing.

Where is this dependency exactly?

[MichalStrehovsky]

RyuJIT depends on never seeing two different CORINFO_MODULE_STRUCT for the same thing.

Where is this dependency exactly?

Per #93843 (comment) it's in GenTreeStrCon equality. I think it's this:

runtime/src/coreclr/jit/gentree.cpp

Lines 2704 to 2710 in 2dc7c31

	case GT_CNS_STR:
	if ((op1->AsStrCon()->gtSconCPX == op2->AsStrCon()->gtSconCPX) &&
	(op1->AsStrCon()->gtScpHnd == op2->AsStrCon()->gtScpHnd))
	{
	return true;
	}
	break;

[jkotas]

     if ((op1->AsStrCon()->gtSconCPX == op2->AsStrCon()->gtSconCPX) && 
         (op1->AsStrCon()->gtScpHnd == op2->AsStrCon()->gtScpHnd)) 

This condition is not a precise check to validate whether two string literals are going produce same string object. It is going to miss on string literals from different modules with JIT, and it is going to miss on string literals from different methods with AOT/R2R. It may be better to abstract this comparison via JIT/EE interface, so that it can be more precise.

In general, JIT depending on module handle equality seems to be an anti-pattern.

[SingleAccretion]

It may be better to abstract this comparison via JIT/EE interface, so that it can be more precise.

That was something I considered. It has the following drawbacks:

1. GenTree::Equals becomes more expensive. It feels questionable for "simple tree equality" to depend on (potentially expensive) Jit-EE calls.
2. VN-based optimizations such as CSE operate on the lowered form of CNS_STR, so it won't move the needle for them.

(As I said in the issue) I don't know what the "right" fix here is. This change seems like a practical option.

[jkotas]

I agree that this change is a practical option. It will work ok as long as nothing depends on the comparison being precise.

[MichalStrehovsky]

I agree that this change is a practical option. It will work ok as long as nothing depends on the comparison being precise.

@dotnet/jit-contrib anyone else has an opinion about this fix? We're working around RyuJIT making assumptions about CORINFO_MODULE_STRUCT* handles. The workaround is not great and the RyuJIT assumptions have other problems enumerated by Jan above. Would a JIT-side fix make more sense?

If not, I'll merge this, but it is quite fragile. A more robust fix on the driver side would be to never purge the ILProvider cache and eat the memory overhead (essentially turn a cache into a memory leak). That one is obviously not great either.

[jakobbotsch]

This fix looks good to me. I agree with @SingleAccretion about the concerns for GenTree:: Compare. This function is meant to be a fast syntactic equality check, it is not meant to be a precise or semantic equality check.

[MichalStrehovsky]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7057987064