Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
sandreenko merged 6 commits into from
Nov 17, 2020
Merged

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

sandreenko merged 6 commits into from
Nov 17, 2020

Conversation

@sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Oct 5, 2020 •
edited
Loading

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
@runfoapp runfoapp bot mentioned this pull request Oct 6, 2020
Closed
@sandreenko sandreenko force-pushed the Arm64Apple-Jit-Part3 branch from 14576d9 to 96cf3c5 Compare October 8, 2020 08:30
@YohDeadfall YohDeadfall mentioned this pull request Oct 16, 2020
Closed
@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
sandreenko
sandreenko commented Oct 30, 2020
View reviewed changes
src/coreclr/src/jit/compiler.h Outdated
Copy link
Contributor Author

@sandreenko sandreenko Oct 30, 2020 •
edited
Loading

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.

sandreenko
sandreenko commented Oct 30, 2020
View reviewed changes
src/coreclr/src/jit/lsraxarch.cpp Outdated
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.

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

sandreenko commented Oct 30, 2020

PTAL @dotnet/jit-contrib

This was referenced Oct 30, 2020
Closed
Closed
@sandreenko
Copy link
Contributor Author

sandreenko commented Nov 10, 2020

ping @dotnet/jit-contrib

@echesakov echesakov self-requested a review November 10, 2020 01:07
CarolEidt
CarolEidt reviewed Nov 11, 2020
View reviewed changes
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.

@sandreenko
Copy link
Contributor Author

sandreenko commented Nov 11, 2020

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Nov 11, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Nov 11, 2020
Closed
@sandreenko
Copy link
Contributor Author

sandreenko commented Nov 14, 2020

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

CarolEidt
CarolEidt approved these changes Nov 14, 2020
View reviewed changes
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.

echesakov
echesakov approved these changes Nov 14, 2020
View reviewed 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

@runfoapp runfoapp bot mentioned this pull request Nov 16, 2020
Closed
@sandreenko sandreenko closed this Nov 16, 2020
@sandreenko sandreenko reopened this Nov 16, 2020
@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
Closed
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.

Reviewers

2 more reviewers

@CarolEidt CarolEidt CarolEidt approved these changes

@echesakov echesakov echesakov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sandreenko @CarolEidt @echesakov