Implement CORINFO_FIELD_INTRINSIC_ZERO and _ISLITTLEENDIAN #116853
Conversation
These has shown up on the runtime startup path
There was a problem hiding this comment.
Pull Request Overview
This PR implements two new CORINFO field intrinsics to support runtime startup behavior. Key updates include:
- Adding a switch-case in EmitStaticFieldAccess for CORINFO_FIELD_INTRINSIC_ZERO, which pushes a null reference.
- Adding logic for CORINFO_FIELD_INTRINSIC_ISLITTLEENDIAN to push a constant reflecting the system’s endianness.
| m_pLastNewIns->data[0] = 0; | ||
| #else | ||
| m_pLastNewIns->data[0] = 1; |
There was a problem hiding this comment.
Consider defining named constants for the literal endianness flag values (e.g., ZERO_VALUE and ONE_VALUE) to improve clarity in the code block handling CORINFO_FIELD_INTRINSIC_ISLITTLEENDIAN.
| m_pLastNewIns->data[0] = 0; | |
| #else | |
| m_pLastNewIns->data[0] = 1; | |
| m_pLastNewIns->data[0] = BIG_ENDIAN_FLAG; | |
| #else | |
| m_pLastNewIns->data[0] = LITTLE_ENDIAN_FLAG; |
Copilot uses AI. Check for mistakes.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
src/coreclr/interpreter/compiler.cpp
Outdated
| #else | ||
| m_pLastNewIns->data[0] = 1; | ||
| #endif | ||
| PushInterpType(InterpTypeI1, NULL); |
There was a problem hiding this comment.
Should it be extended to InterpTypeI4 on the eval stack even though it is a bool?
There was a problem hiding this comment.
This is actually one thing I am not sure about, maybe @BrzVlad could provide a guidance here.
There was a problem hiding this comment.
We only support pushing I4,I8,R4,R8 or VT on the stack. When pushing on the stack from IL locals/args we will sign extend. In this particular example, whether you push InterpTypeI1 or InterpTypeI4, you will still end up pushing an I4 var so there is not really a difference in practice. Overall I suggest pushing the I4 explicitly. In the future we will probably get rid of the StackType* so we will have to push the actual type anyway.
There was a problem hiding this comment.
Thank you, I'll change it that way then.
| EmitStind(interpFieldType, pFieldInfo->structType, 0, true); | ||
| switch (pFieldInfo->fieldAccessor) | ||
| { | ||
| case CORINFO_FIELD_INTRINSIC_ZERO: |
There was a problem hiding this comment.
CORINFO_FIELD_INTRINSIC_EMPTY_STRING is handled in EmitStaticFieldAddress.
Should it be all together?
There was a problem hiding this comment.
My judgment call was that static field values and addresses should be separate things, so that's why I split it originally. I'm not opposed to unifying them into one function, but 'EmitStaticFieldAddress' is followed by a ldind, so that gets awkward if you want to generate a literal zero.
There was a problem hiding this comment.
It was actually me who put the CORINFO_FIELD_INTRINSIC_EMPTY_STRING in the EmitStaticFieldAddress. I guess I might have done it in an unnecessary complicated manner by getting the address and then emiting the indirection opcode.
There was a problem hiding this comment.
But I have thought the indirection was necessary.
There was a problem hiding this comment.
The extra indirection for CORINFO_FIELD_INTRINSIC_EMPTY_STRING looks suspect to me. emptyStringLiteral returns the string object (for IAT_VALUE). There should not be any indirection required.
These has shown up on the runtime startup path