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

JIT: Sign/zero-extend small primitive arguments for pinvokes on arm32 and Apple arm64 #106314

Merged
merged 11 commits into from
Aug 14, 2024

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Aug 13, 2024

Fix #101046

Alternative to #106312 that keeps the behavior as is for the managed calling convention and only changes it for pinvokes. This requires changing fcalls in the VM to access small primitive typed arguments without assumptions about normalization.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 13, 2024
@@ -293,14 +293,11 @@ typedef uint8_t CODE_LOCATION;

typedef bool CLR_BOOL;
Copy link
Member

Choose a reason for hiding this comment

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

Delete?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it's still used as the return value for HandleTableRestrictedCallbackFunction.

Copy link
Member

Choose a reason for hiding this comment

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

And TryGetTaggedMemoryCallback.

These correspond to the following managed methods:

CLR_BOOL is not quite right for neither of these, but I do not think that this mismatch is going to cause any real problem. It would be nice to clean this up in a follow up.

src/tests/issues.targets Outdated Show resolved Hide resolved
@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Aug 14, 2024
@JulieLeeMSFT
Copy link
Member

@AndyAyersMS, PTAL. This is for RC1 snap.

@jakobbotsch jakobbotsch marked this pull request as ready for review August 14, 2024 18:17
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS @jkotas

Minor diffs

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Jit changes LGTM.

@@ -1335,7 +1335,10 @@ typedef UINT32 FC_UINT8_RET;
typedef INT32 FC_INT16_RET;
typedef UINT32 FC_UINT16_RET;

// Small primitive args are not widened.
Copy link
Member

Choose a reason for hiding this comment

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

The comment around line 150 in this file still mentions CLR_BOOL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mind if I update that in a separate change tomorrow?

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, updated it as well now due to those other places I missed the conversion.

@jkotas
Copy link
Member

jkotas commented Aug 14, 2024

These places are missing the CLR_BOOL -> FC_BOOL_ARG conversion:

\runtime\src\coreclr\vm\comutilnative.h(43):static FCDECL3(StringObject *, StripFileInfo, Object *orefExcepUNSAFE, StringObject *orefStrUNSAFE, CLR_BOOL isRemoteStackTrace); - dead code, shoul be deleted
\runtime\src\coreclr\vm\debugdebugger.cpp(236):CLR_BOOL fNeedFileInfo,
\runtime\src\coreclr\vm\debugdebugger.h(155):CLR_BOOL fNeedFileInfo,

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. Thanks!

@JulieLeeMSFT
Copy link
Member

JulieLeeMSFT commented Aug 14, 2024

cc @carlossanlop to include this in RC1 snap if CI tests pass.

@carlossanlop
Copy link
Member

Thanks. I'm aware, it's in my list of PRs to wait. @jakobbotsch reached out earlier.

@jakobbotsch
Copy link
Member Author

The runtime pipeline is done (some of the jobs didn't have their status updates, presumably because of the outage). Build analysis is happy. We can skip waiting for the superpmi jobs – they were green in the previous run and there hasn't been any JIT changes since then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: Small primitives on arm32/apple arm64 ABI need explicit normalization to 32 bits
5 participants