Skip to content
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

Issue-475 : Using the unsigned operations where it s available on PrimitiveType #603

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

PapisKang
Copy link
Contributor

Fixes #475

On PrimitiveType.cs, PrimitiveTypeKind.UnsignedShort and PrimitiveTypeKind.UnsignedInt was using signed load operations.

On this pull request, i ve updated those operations to unsigned methods.

Base on tests i ve made in C#
Sign-Extension is used when loading a signed value
Zero-Extension when loading an unsigned.

Unsigned types are loaded with ldind.u* operations.

sbyte[] signedBytes = { -1 };
byte[] unsignedBytes = { 255 };

sbyte signedResult = signedBytes[0];
byte unsignedResult = unsignedBytes[0];

int signedInt = (int)signedResult;
int unsignedInt = (int)unsignedResult;

image

@ForNeVeR ForNeVeR self-assigned this Jun 15, 2024
@ForNeVeR ForNeVeR self-requested a review June 15, 2024 12:03
{ PrimitiveTypeKind.Int, (OpCodes.Ldind_I4, OpCodes.Stind_I4) },
{ PrimitiveTypeKind.UnsignedInt, (OpCodes.Ldind_I4, OpCodes.Stind_I4) },
{ PrimitiveTypeKind.UnsignedInt, (OpCodes.Ldind_U4, OpCodes.Stind_I4) },
{ PrimitiveTypeKind.Float, (OpCodes.Ldind_R4, OpCodes.Stind_R4) },
{ PrimitiveTypeKind.Long, (OpCodes.Ldind_I8, OpCodes.Stind_I8) },
{ PrimitiveTypeKind.UnsignedLong, (OpCodes.Ldind_I8, OpCodes.Stind_I8) },
Copy link
Owner

Choose a reason for hiding this comment

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

There's Ldind_U8 as well, can you update this one as well? Other than this, LGTM, thanks for fix and investigation!

Copy link
Contributor

Choose a reason for hiding this comment

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

ldind_u8 does not exist.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I wasn't attentive enough when reading the instruction list. I double-checked it is present in the list, but missed the fact it's declared as an alias for ldind.i8.
image

Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Thanks!

@ForNeVeR ForNeVeR merged commit b32f8ae into ForNeVeR:main Jun 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange approach to ldind
3 participants