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.

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

Cranelift: shrink ABIArgSlot #8163

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −18
Open

Cranelift: shrink ABIArgSlot #8163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions cranelift/codegen/src/isa/aarch64/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
Some((ty, slot_offset))
})
.map(|(ty, offset)| ABIArgSlot::Stack {
offset,
offset: i32::try_from(offset).unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't currently check that the stack frame size is less than 2GB, so this unwrap can panic: #7916

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know! I'll hold off on merging this until I have a chance to think more about that or somebody else fixes it.

ty,
extension: param.extension,
})
Expand Down Expand Up @@ -1325,7 +1325,7 @@ where
let offset = i64::from(*next_stack);
*next_stack += ty.bytes();
ABIArgSlot::Stack {
offset,
offset: i32::try_from(offset).unwrap(),
ty,
extension: ir::ArgumentExtension::None,
}
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/riscv64/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ impl ABIMachineSpec for Riscv64MachineDeps {
debug_assert!(size.is_power_of_two());
next_stack = align_to(next_stack, size);
slots.push(ABIArgSlot::Stack {
offset: next_stack as i64,
offset: i32::try_from(next_stack).unwrap(),
ty: *reg_ty,
extension: param.extension,
});
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/s390x/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ impl ABIMachineSpec for S390xMachineDeps {
let offset = (next_stack + offset) as i64;
next_stack += slot_size;
ABIArgSlot::Stack {
offset,
offset: i32::try_from(offset).unwrap(),
ty: param.value_type,
extension: param.extension,
}
Expand Down
4 changes: 2 additions & 2 deletions cranelift/codegen/src/isa/x64/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ impl ABIMachineSpec for X64ABIMachineSpec {
None => {
next_stack = align_to(next_stack, 8) + 8;
ABIArgSlot::Stack {
offset: (next_stack - 8) as i64,
offset: i32::try_from(next_stack - 8).unwrap(),
ty: ir::types::I64,
extension: param.extension,
}
Expand Down Expand Up @@ -250,7 +250,7 @@ impl ABIMachineSpec for X64ABIMachineSpec {
debug_assert!(size.is_power_of_two());
next_stack = align_to(next_stack, size);
slots.push(ABIArgSlot::Stack {
offset: next_stack as i64,
offset: i32::try_from(next_stack).unwrap(),
ty: *reg_ty,
extension: param.extension,
});
Expand Down
25 changes: 14 additions & 11 deletions cranelift/codegen/src/machinst/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ pub enum ABIArgSlot {
/// Arguments only: on stack, at given offset from SP at entry.
Stack {
/// Offset of this arg relative to the base of stack args.
offset: i64,
offset: i32,
/// Value type of this arg.
ty: ir::Type,
/// Should this arg be zero- or sign-extended?
Expand All @@ -176,10 +176,12 @@ impl ABIArgSlot {
}
}

/// A vector of `ABIArgSlot`s. Inline capacity for one element because basically
/// 100% of values use one slot. Only `i128`s need multiple slots, and they are
/// super rare (and never happen with Wasm).
pub type ABIArgSlotVec = SmallVec<[ABIArgSlot; 1]>;
/// A vector of `ABIArgSlot`s. Inline capacity for two elements because that
/// many fit inline in a SmallVec anyway. Basically 100% of values use one slot.
/// Only `i128`s need two slots, and they are super rare (and never happen with
/// Wasm). On 64-bit targets, nothing needs more than two slots, so this should
/// never spill to the heap.
pub type ABIArgSlotVec = SmallVec<[ABIArgSlot; 2]>;

/// An ABIArg is composed of one or more parts. This allows for a CLIF-level
/// Value to be passed with its parts in more than one location at the ABI
Expand Down Expand Up @@ -257,7 +259,7 @@ impl ABIArg {
) -> ABIArg {
ABIArg::Slots {
slots: smallvec![ABIArgSlot::Stack {
offset,
offset: i32::try_from(offset).unwrap(),
ty,
extension,
}],
Expand Down Expand Up @@ -1523,7 +1525,7 @@ impl<M: ABIMachineSpec> Callee<M> {
};
insts.push(M::gen_load_stack(
StackAMode::FPOffset(
M::fp_to_arg_offset(self.call_conv, &self.flags) + offset,
M::fp_to_arg_offset(self.call_conv, &self.flags) + i64::from(offset),
ty,
),
*into_reg,
Expand Down Expand Up @@ -1577,7 +1579,8 @@ impl<M: ABIMachineSpec> Callee<M> {
let addr_reg = self.arg_temp_reg[idx].unwrap();
insts.push(M::gen_load_stack(
StackAMode::FPOffset(
M::fp_to_arg_offset(self.call_conv, &self.flags) + offset,
M::fp_to_arg_offset(self.call_conv, &self.flags)
+ i64::from(offset),
ty,
),
addr_reg,
Expand Down Expand Up @@ -2433,7 +2436,7 @@ impl<M: ABIMachineSpec> CallSite<M> {
};
locs.push((
data.into(),
ArgLoc::Stack(StackAMode::SPOffset(offset, ty)),
ArgLoc::Stack(StackAMode::SPOffset(i64::from(offset), ty)),
));
}
}
Expand All @@ -2459,7 +2462,7 @@ impl<M: ABIMachineSpec> CallSite<M> {
ABIArgSlot::Reg { reg, .. } => ArgLoc::Reg(reg.into()),
ABIArgSlot::Stack { offset, .. } => {
let ty = M::word_type();
ArgLoc::Stack(StackAMode::SPOffset(offset, ty))
ArgLoc::Stack(StackAMode::SPOffset(i64::from(offset), ty))
}
};
locs.push((tmp.into(), loc));
Expand Down Expand Up @@ -2567,7 +2570,7 @@ impl<M: ABIMachineSpec> CallSite<M> {
sig_data.sized_stack_arg_space()
};
insts.push(M::gen_load_stack(
StackAMode::SPOffset(offset + ret_area_base, ty),
StackAMode::SPOffset(i64::from(offset) + ret_area_base, ty),
*into_reg,
ty,
));
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/prelude_lower.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@
(ty Type)
(extension ArgumentExtension))
(Stack
(offset i64)
(offset i32)
(ty Type)
(extension ArgumentExtension))))

Expand Down