Make getTypeForPrimitiveValueClass treat different signs as different types #71633
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFixes #71632.
|
MichalPetryka
changed the title
src/coreclr/vm/jitinterface.cpp
Outdated
| @@ -3969,54 +3969,13 @@ CorInfoType CEEInfo::getTypeForPrimitiveValueClass( | |||
| } | |||
| else | |||
| { | |||
| switch (th.GetInternalCorElementType()) | |||
| CorElementType elementType = th.GetInternalCorElementType(); | |||
| if (elementType >= ELEMENT_TYPE_BOOLEAN && elementType <= ELEMENT_TYPE_R8 | |||
There was a problem hiding this comment.
We have a helper method for this. I think it should be possible to simplify the whole thing into:
TypeHandle th(clsHnd);
_ASSERTE (!th.IsGenericVariable());
CorElementType elementType = th.GetVerifierCorElementType();
if (CorIsPrimitiveType(elementtype) || elementType == ELEMENT_TYPE_PTR || ELEMENT_TYPE_FNPTR)
{
result = asCorInfoType(elementType);
}
It looks odd that this method is handling ELEMENT_TYPE_PTR and ELEMENT_TYPE_FNPTR. It does not match the name getTypeForPrimitiveValueClass.
Is the JIT really expecting this method to handle ELEMENT_TYPE_PTR and ELEMENT_TYPE_FNPTR?
There was a problem hiding this comment.
Is the JIT really expecting this method to handle
ELEMENT_TYPE_PTRandELEMENT_TYPE_FNPTR?
Yes, for example in https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L4203
There was a problem hiding this comment.
CorIsPrimitiveType would also include ELEMENT_TYPE_END, would that be okay?
There was a problem hiding this comment.
Yes, for example in https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L4203
I do not see the connection between this line and the method being changed in this PR. Could you please shed some light on it?
CorIsPrimitiveType would also include ELEMENT_TYPE_END, would that be okay?
Yes, that' ok. ELEMENT_TYPE_END is never returned by as CorElementType of a type.
There was a problem hiding this comment.
I do not see the connection between this line and the method being changed in this PR. Could you please shed some light on it?
Right, wrong line, here: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L6586
There was a problem hiding this comment.
I'll try it after the CI finishes, want to see if everything's okay with the current changes.
There was a problem hiding this comment.
@jkotas Could you take a look if the musl failure that appeared after removing the pointers looks related?
There was a problem hiding this comment.
The failure looks like the same as #66413 (comment) which might be related to #71439. Does not look related to your change.
There was a problem hiding this comment.
The failure is unrelated and failed in other PRs - #66413 (comment)
There was a problem hiding this comment.
I have opened blocking-clean-in issue #71684 on this.
When merging on red, failures should be always tracked by active blocking-clean-in issue.
|
cc @dotnet/jit-contrib PTAL for this community PR. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #71632.