Skip to content

machinst ABI: Pass fixed frame size to gen_clobber_restore#2346

Merged
cfallin merged 1 commit intobytecodealliance:mainfrom
uweigand:abi-noframepointer
Nov 3, 2020
Merged

machinst ABI: Pass fixed frame size to gen_clobber_restore#2346
cfallin merged 1 commit intobytecodealliance:mainfrom
uweigand:abi-noframepointer

Conversation

@uweigand
Copy link
Member

@uweigand uweigand commented Nov 2, 2020

The ABI common code currently passes the fixed frame size to
the gen_clobber_save back-end routine, which is required to
emit code to allocate the required stack space in the prologue.

Similarly, the back-end needs to emit code to de-allocate the
stack in the epilogue. However, at this point the back-end
does not have access to that fixed frame size value any more.
With targets that use a frame pointer, this does not matter,
since de-allocation can be done simply by assigning the frame
pointer back to the stack pointer. However, on targets that
do not use a frame pointer, the frame size is required.

To allow back-ends that option, this patch changes ABI common
code to pass the fixed frame size to get_clobber_restore as
well (computed identically to how it is done for get_clobber_save).

@uweigand
Copy link
Member Author

uweigand commented Nov 2, 2020

@cfallin And this is the other change discussed here #2128 (comment) (required for the IBM Z back-end).

Comment on lines +989 to +992
let mut fixed_frame_storage_size = 0;
if !self.call_conv.extends_baldrdash() {
fixed_frame_storage_size += self.frame_size();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut fixed_frame_storage_size = 0;
if !self.call_conv.extends_baldrdash() {
fixed_frame_storage_size += self.frame_size();
}
let fixed_frame_storage_size = if !self.call_conv.extends_baldrdash() {
self.frame_size()
} else {
0
};

Also why this condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you look at the code in gen_prologue above, it passes a fixed_frame_storage_size of 0 to gen_clobber_save with the baldrdash calling convention. For consistency, we need to do the same for gen_clobber_restore.

Likewise, I followed the same coding convention with the "mut fixed_frame_storage_size" as is currently in place in gen_prologue. Given that any future changes to the computation of the size will need to be kept in sync between gen_prologue and gen_epilogue, it seems preferable to me to also have the code itself as similar as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to simply store the fixed_frame_storage_size in self -- we get a mutable self in gen_prologue so we should be able to write it there. This avoids the duplication of logic and potential for mismatches...

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, updated the patch accordingly.

@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 Nov 2, 2020
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks! As noted below, I think there may be a better way to factor things, but it's a sensible change overall.

Comment on lines +989 to +992
let mut fixed_frame_storage_size = 0;
if !self.call_conv.extends_baldrdash() {
fixed_frame_storage_size += self.frame_size();
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to simply store the fixed_frame_storage_size in self -- we get a mutable self in gen_prologue so we should be able to write it there. This avoids the duplication of logic and potential for mismatches...

The ABI common code currently passes the fixed frame size to
the gen_clobber_save back-end routine, which is required to
emit code to allocate the required stack space in the prologue.

Similarly, the back-end needs to emit code to de-allocate the
stack in the epilogue.  However, at this point the back-end
does not have access to that fixed frame size value any more.
With targets that use a frame pointer, this does not matter,
since de-allocation can be done simply by assigning the frame
pointer back to the stack pointer.  However, on targets that
do not use a frame pointer, the frame size is required.

To allow back-ends that option, this patch changes ABI common
code to pass the fixed frame size to get_clobber_restore as
well (the same value as is passed to get_clobber_save).
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks!

@cfallin cfallin merged commit 0c24099 into bytecodealliance:main Nov 3, 2020
@uweigand uweigand deleted the abi-noframepointer branch November 10, 2020 12:14
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.

3 participants