[clr-interp] Emit INTOP_NEWMDARR only for array ctor #116544
Conversation
There was a problem hiding this comment.
Pull Request Overview
The PR improves the interpreter logic to ensure that INTOP_NEWMDARR is only emitted for array constructors, using the array rank as the dimension count rather than the original approach.
- Removed fetching of method signature and constructor class.
- Now retrieves the array class handle and its rank, and validates the constructor call accordingly.
Comments suppressed due to low confidence (1)
src/coreclr/interpreter/compiler.cpp:2514
- Consider adding a brief comment above this conditional block explaining that the rank value is used as the expected number of constructor arguments for multi-dimensional arrays, which would improve clarity and maintainability.
if (callInfo.sig.retType == CORINFO_TYPE_VOID && callInfo.sig.numArgs == rank && (m_compHnd->getMethodAttribs(resolvedCallToken.hMethod) & CORINFO_FLG_CONSTRUCTOR) != 0)
github-actions
bot
added
the
needs-area-label
kotlarmilos
added
area-CodeGen-Interpreter-coreclr
and removed
needs-area-label
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
src/coreclr/interpreter/compiler.cpp
Outdated
| @@ -2508,15 +2508,15 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* constrainedClass, bool rea | |||
| } | |||
| else if ((callInfo.classFlags & CORINFO_FLG_ARRAY) && !readonly) | |||
There was a problem hiding this comment.
| else if ((callInfo.classFlags & CORINFO_FLG_ARRAY) && !readonly) | |
| else if (callInfo.classFlags & CORINFO_FLG_ARRAY) |
!readonly should not be needed here
There was a problem hiding this comment.
Instead, you can check for newobj opcode and then you do not need the condition below:
if ((callInfo.classFlags & CORINFO_FLG_ARRAY) && newObj)
There was a problem hiding this comment.
(It is what RuyJIT does - look for impImportNewObjArray.)
src/coreclr/interpreter/compiler.cpp
Outdated
| AddIns(INTOP_NEWMDARR); | ||
| m_pLastNewIns->data[0] = GetDataItemIndex(ctorClass); | ||
| m_pLastNewIns->data[1] = numArgs; | ||
| if (callInfo.sig.retType == CORINFO_TYPE_VOID && callInfo.sig.numArgs == rank && (m_compHnd->getMethodAttribs(resolvedCallToken.hMethod) & CORINFO_FLG_CONSTRUCTOR) != 0) |
There was a problem hiding this comment.
callInfo.sig.numArgs == rank condition is not 100% right. The multi-dimm array constructors can take rank or rank * 2 arguments.
The overload that takes rank * 2 arguments is not expressible in C#. It allows you to specify lower bounds.
src/coreclr/interpreter/compiler.cpp
Outdated
| m_pLastNewIns->data[0] = GetDataItemIndex(ctorClass); | ||
| m_pLastNewIns->data[1] = numArgs; | ||
| m_pLastNewIns->data[0] = GetDataItemIndex(arrayClsHnd); | ||
| m_pLastNewIns->data[1] = rank; |
There was a problem hiding this comment.
This should not be rank. This should be the actual number of the arguments in the signature, so that the code handles different overloads of array constructor correctly. The earlier code looks correct, the new code looks wrong. I think this part of the change should be reverted.
There was a problem hiding this comment.
Also, you can rename the argument name on the executor side to numArgs to avoid confusion
runtime/src/coreclr/vm/interpexec.cpp
Line 135 in ca63ce6
There was a problem hiding this comment.
This should be the actual number of the arguments in the signature, so that the code handles different overloads of array constructor correctly.
Sorry I overlooked this even though you mentioned it. I was focused on #116493 and I think the error was not properly guarding the INTOP_NEWMDARR case, which caused other instance methods to take the same path.
Co-authored-by: Jan Kotas <[email protected]>
Co-authored-by: Jan Kotas <[email protected]>
Description
This PR improves condition to emit INTOP_NEWMDARR only for array ctor and use rank as dimension count.
Fixes #116493