Skip to content

[clr-interp] Implement support for multi-dimensional array get and set accessors #116483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

kotlarmilos
Copy link
Member

Description

This PR adds support for multi-dimensional array get and set accessors.

Changes

  • Implemented LDELEMA_MD
  • Implemented MDArray.Get as a LDELEMA_MD + LDOBJ
  • Implemented MDArray.Set as a LDELEMA_MD + STOBJ

Copy link
Contributor

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

@kotlarmilos kotlarmilos requested a review from jkotas June 10, 2025 16:06
@kotlarmilos kotlarmilos mentioned this pull request Jun 10, 2025
61 tasks
int32_t idx = *(int32_t*)(stack + ip[2] + (i + 1) * 8);
if (idx < lowerBound || idx >= lowerBound + dimLen)
COMPlusThrow(kIndexOutOfRangeException);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to do the same array covariance type checks as single-dimensional arrays.

For example, the following should fail with ArrayTypeMismatchException

string[,] a = new string[1,1];
object[,] ao = a;

ref object r = ref ao[0,0];
Console.WriteLine(r);

@jkotas
Copy link
Member

jkotas commented Jun 10, 2025

Multi-dimensional array get and set accessors are regular methods that you should be able to just call and the runtime will provide implementations for them. The special opcodes to implement them are an optimization. I do not have a problem with building this optimization, just want to make sure that it is intentional.

@jkotas
Copy link
Member

jkotas commented Jun 10, 2025

Implemented MDArray.Get as a LDELEMA_MD + LDOBJ
Implemented MDArray.Set as a LDELEMA_MD + STOBJ

This won't work for array of object references due to co-variance.

@kotlarmilos
Copy link
Member Author

Multi-dimensional array get and set accessors are regular methods that you should be able to just call and the runtime will provide implementations for them. The special opcodes to implement them are an optimization. I do not have a problem with building this optimization, just want to make sure that it is intentional.

Thanks. At first, I thought this was an intrinsic (a method without metadata), since m_compHnd->getMethodNameFromMetadata returned null. This PR introduces a fast path while fcalls will be added in #115916.

@jkotas
Copy link
Member

jkotas commented Jun 11, 2025

This PR introduces a fast path while fcalls will be added in #115916.

Why do we need both? If you add the intrinsic expansion, we won't need the handle these as calls.

Nit: These are not fcalls. It is one method implemented in IL calling another method implemented in IL.

@kotlarmilos
Copy link
Member Author

kotlarmilos commented Jun 11, 2025

This PR introduces a fast path while fcalls will be added in #115916.

Why do we need both? If you add the intrinsic expansion, we won't need the handle these as calls.

Nit: These are not fcalls. It is one method implemented in IL calling another method implemented in IL.

Are there any scenarios that the intrinsic expansion doesn’t support? For example, you mentioned the covariance check in this and previous PRs which I wasn’t aware of.

/cc: @kg

@jkotas
Copy link
Member

jkotas commented Jun 11, 2025

Are there any scenarios that the intrinsic expansion doesn’t support?

There is no functional difference between the two. Intrinsic expansion is just a performance optimization.

You can do method calls for all cases, you can do intrinsic expansion for all cases, or you can do a mix of both (it is what the optimizing JIT does - it does the intrinsic expansion but only when optimizations are on and only for the simple cases.)

@jkotas
Copy link
Member

jkotas commented Jun 11, 2025

If I were to choose, I would do just calls for now (ie what #115916 does).

@kotlarmilos
Copy link
Member Author

kotlarmilos commented Jun 11, 2025

Ok, we can review this once we introduce optimizations. Closing this in favor of #115916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants