Skip to content

cranelift: Simplify StackAMode variants#8316

Merged
jameysharp merged 1 commit intobytecodealliance:mainfrom
jameysharp:untyped-stack-amode
Apr 9, 2024
Merged

cranelift: Simplify StackAMode variants#8316
jameysharp merged 1 commit intobytecodealliance:mainfrom
jameysharp:untyped-stack-amode

Conversation

@jameysharp
Copy link
Contributor

Instead of describing how these address modes might be implemented (using a stack or frame pointer, say), describe what they represent. Also, remove the unused type fields from all variants and from gen_get_stack_addr.

The three kinds of stack addresses that we currently need to generate in a target independent way refer to either caller or callee argument areas, or refer to the spill slots and explicit stack slots within the frame.

It is a target-specific implementation detail whether the incoming argument area is indexed relative to the frame pointer, or whether the stack slots are located by tracking a "nominal" stack pointer offset.

So let's not muddy up the target-independent code by using names that refer to these target-specific concepts.

Instead of describing how these address modes might be implemented
(using a stack or frame pointer, say), describe what they represent.
Also, remove the unused type fields from all variants and from
`gen_get_stack_addr`.

The three kinds of stack addresses that we currently need to generate in
a target independent way refer to either caller or callee argument
areas, or refer to the spill slots and explicit stack slots within the
frame.

It is a target-specific implementation detail whether the incoming
argument area is indexed relative to the frame pointer, or whether the
stack slots are located by tracking a "nominal" stack pointer offset.

So let's not muddy up the target-independent code by using names that
refer to these target-specific concepts.
@jameysharp jameysharp requested a review from elliottt April 8, 2024 18:50
@jameysharp jameysharp requested a review from a team as a code owner April 8, 2024 18:50
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels Apr 8, 2024
Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Nice!

@jameysharp jameysharp added this pull request to the merge queue Apr 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 8, 2024
@jameysharp
Copy link
Contributor Author

Looks to me like maybe a flaky test in the preview2 test suite, so I'm going to try re-submitting it and see if it goes through this time.

@jameysharp jameysharp added this pull request to the merge queue Apr 9, 2024
Merged via the queue into bytecodealliance:main with commit 9fd00a0 Apr 9, 2024
@jameysharp jameysharp deleted the untyped-stack-amode branch April 9, 2024 01:21
jameysharp added a commit to jameysharp/wasmtime that referenced this pull request Apr 18, 2024
This reverts the key parts of e3a08d4
(bytecodealliance#8151), because it turns out that we didn't need that abstraction.

Several changes in the last month have enabled this:

- bytecodealliance#8292 and then bytecodealliance#8316 allow us to refer to either incoming or outgoing
  argument areas in a (mostly) consistent way

- bytecodealliance#8327, bytecodealliance#8377, and bytecodealliance#8383 demonstrate that we never need to delay
  writing stack arguments directly to their final location
jameysharp added a commit to jameysharp/wasmtime that referenced this pull request Apr 18, 2024
This reverts the key parts of e3a08d4
(bytecodealliance#8151), because it turns out that we didn't need that abstraction.

Several changes in the last month have enabled this:

- bytecodealliance#8292 and then bytecodealliance#8316 allow us to refer to either incoming or outgoing
  argument areas in a (mostly) consistent way

- bytecodealliance#8327, bytecodealliance#8377, and bytecodealliance#8383 demonstrate that we never need to delay
  writing stack arguments directly to their final location

prtest:full
github-merge-queue bot pushed a commit that referenced this pull request Apr 18, 2024
This reverts the key parts of e3a08d4
(#8151), because it turns out that we didn't need that abstraction.

Several changes in the last month have enabled this:

- #8292 and then #8316 allow us to refer to either incoming or outgoing
  argument areas in a (mostly) consistent way

- #8327, #8377, and #8383 demonstrate that we never need to delay
  writing stack arguments directly to their final location

prtest:full
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants