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

arm64 osx: support byte sizes from lowering to codegen. #43024

Merged
merged 6 commits into from
Nov 17, 2020

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Oct 5, 2020

Keep precise byte sizes and offsets for call arguments from lowering to codegen. It is a continuation of #42503.

Contributes to #41130.

No diffs.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 5, 2020
@sandreenko sandreenko force-pushed the Arm64Apple-Jit-Part3 branch from 3899ec9 to 14576d9 Compare October 5, 2020 20:31
@sandreenko sandreenko force-pushed the Arm64Apple-Jit-Part3 branch from 14576d9 to 96cf3c5 Compare October 8, 2020 08:30
@sandreenko sandreenko force-pushed the Arm64Apple-Jit-Part3 branch from 96cf3c5 to 4602f28 Compare October 30, 2020 18:16
@sandreenko sandreenko marked this pull request as ready for review October 30, 2020 19:45
// Get the number of bytes that this argument is occupying on the stack.
// Get the number of bytes that this argument is occupying on the stack,
// including padding up to the target pointer size for platforms
// where a stack argument can't take less.
Copy link
Contributor Author

@sandreenko sandreenko Oct 30, 2020

Choose a reason for hiding this comment

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

it could be confusing when we have arguments that require more than a pointer size, for example:
call(arguments to occupy registers, byte a, struct with 16 bytes alignment b)
in this example we will have such offsets on arm64 not-apple:

a - offset 0, GetStackByteSize() = 8,
b - offset 16, GetStackByteSize() = 16.

so the space between offset 1 and 8 is padding for a, the space between 8 and 16 is an alignment for b.

if (!layout->HasGCPtr() && (size & (XMM_REGSIZE_BYTES - 1)) != 0)
{
regMaskTP regMask = allRegs(TYP_INT);

#ifdef TARGET_X86
if ((size % 2) != 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

size was putArgStk->gtNumSlots * TARGET_POINTER_SIZE where TARGET_POINTER_SIZE % 2 == 0 so that condition was always false.

@sandreenko sandreenko force-pushed the Arm64Apple-Jit-Part3 branch from 4602f28 to 703636e Compare October 30, 2020 21:56
@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib

@sandreenko
Copy link
Contributor Author

ping @dotnet/jit-contrib

@echesakov echesakov self-requested a review November 10, 2020 01:07
Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

It's been a while since I reviewed the previous change; I just have a few questions and comments for now.

src/coreclr/src/jit/lclvars.cpp Outdated Show resolved Hide resolved
src/coreclr/src/jit/gentree.h Outdated Show resolved Hide resolved
src/coreclr/src/jit/gentree.h Show resolved Hide resolved
src/coreclr/src/jit/gentree.h Show resolved Hide resolved
@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sandreenko
Copy link
Contributor Author

The PR was updated, the failures in stress modes are not relevant.

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM - it would be great if @echesakovMSFT could also re-review; there are a lot of somewhat tricky changes.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

Looks good, left some comments about code style

src/coreclr/src/jit/codegenarm64.cpp Outdated Show resolved Hide resolved
src/coreclr/src/jit/codegenarmarch.cpp Outdated Show resolved Hide resolved
src/coreclr/src/jit/gentree.h Outdated Show resolved Hide resolved
@sandreenko sandreenko merged commit 552da07 into dotnet:master Nov 17, 2020
@sandreenko sandreenko deleted the Arm64Apple-Jit-Part3 branch November 17, 2020 05:38
@sandreenko sandreenko mentioned this pull request Dec 2, 2020
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2020
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.

3 participants