More general VN const casting

[BruceForstall]

When creating a VN cast, skip the cast if the target type is any GC type (not just TYP_BYREF) if the source operand is any TYP_I_IMPL constant (not just handle constant). We can lose track of whether a value is a handle due to shared const CSE.

[ghost]

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

Issue Details

---

When creating a VN cast, skip the cast if the target type is any GC type (not just TYP_BYREF) if the source operand is any TYP_I_IMPL constant (not just handle constant). We can lose track of whether a value is a handle due to shared const CSE.

Author:	BruceForstall
Assignees:	BruceForstall
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

I think we should disallow these kinds of mismatches. The implicit TYP_BYREF <-> TYP_I_IMPL conversions in JIT IR presumably only exist as an artifact of implicit TYP_I_IMPL -> TYP_BYREF conversions in IL. I don't think we should allow this to be a thing for TYP_REF <-> TYP_I_IMPL in JIT IR.

[BruceForstall]

The implicit TYP_BYREF <-> TYP_I_IMPL conversions in JIT IR presumably only exist as an artifact of implicit TYP_I_IMPL -> TYP_BYREF conversions in IL

Not in the case I'm looking at. If we have a constant JIT-time handle (pointer to a frozen segment string object, maybe?) then we treat that as a constant with a "handle" attribute in VN. We can take this constant and constant propagate it to TYP_REF uses. We can also lose track of it being a handle due to "shared const CSE", where we pick a base constant X and then construct X + C as the actual constant, which has no place to hang the "handle" flags (on the ADD).

Maybe we shouldn't create these type mismatches during constant prop, etc.

This fix is actually a smaller, more targeted fix, than the other change I had in draft form #94330. This one seems slightly better -- it's avoiding creating a VNF_Cast between TYP_I_IMPL and TYP_REF, which the VN cast code (deeper down) doesn't like. Seems like a reasonable extension of the BYREF exception that already exists.

If we wanted to actually outlaw these kind of type mismatches, we'd have to do a bunch of work in assertion prop and CSE to avoid creating them (and also add some kind of type checking to ensure they don't get created).

[jakobbotsch]

Not in the case I'm looking at. If we have a constant JIT-time handle (pointer to a frozen segment string object, maybe?) then we treat that as a constant with a "handle" attribute in VN. We can take this constant and constant propagate it to TYP_REF uses. We can also lose track of it being a handle due to "shared const CSE", where we pick a base constant X and then construct X + C as the actual constant, which has no place to hang the "handle" flags (on the ADD).

The VN code here is allowing implicit TYP_I_IMPL -> TYP_BYREF. That's more reasonable to me since we allow implicit TYP_I_IMPL <-> TYP_BYREF conversion in JIT IR. I do not think we allow the same for TYP_I_IMPL <-> TYP_REF.

If we wanted to actually outlaw these kind of type mismatches, we'd have to do a bunch of work in assertion prop and CSE to avoid creating them (and also add some kind of type checking to ensure they don't get created).

Yes, I think we should outlaw this, and treat places that create them as creating illegal IR... having these kinds of implicit conversions make things really confusing when reasoning about the IR. I very much doubt that all places in the JIT are correctly handling TYP_REF and TYP_I_IMPL as if they are implicitly convertible to each other, so trying to make this one place in VN handle it does not seem right.

It sounds like we have a few bugs around this, which is not surprising given that the frozen object constants are very new. For one, I think VN should not be giving two differently typed trees the same VN, since they are by definition not equal if they have different types (maybe except the TYP_I_IMPL <-> TYP_BYREF case, due to allowed implicit conversion in JIT IR).

[SingleAccretion]

It sounds to me like not typing handles properly is causing some (new, with the frozen objects) issues; it should be simple to fix by passing the right type when creating the handle VN instead of always using I_IMPL.

The assertion prop issue could be solvable in a similar manner I think. We should create TYP_REF trees in gtNewIconHandle for frozen object handles.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.