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

Correct handling string return values #1292

Merged
merged 4 commits into from
Apr 21, 2022
Merged

Correct handling string return values #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf59967
Correct handling string return values
mohammadfawaz Apr 13, 2022
422697b
Merge branch 'master' into mohammadfawaz/strings_in_ir
mohammadfawaz Apr 18, 2022
e55b4c5
Fix tests
mohammadfawaz Apr 19, 2022
fa493ba
Merge branch 'master' into mohammadfawaz/strings_in_ir
mohammadfawaz Apr 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions sway-core/src/asm_generation/from_ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,11 @@ impl<'ir> AsmBuilder<'ir> {
self.ptr_map.insert(*ptr, Storage::Stack(stack_base));
stack_base += 4;
}
Type::String(_) => {
Type::String(n) => {
// Strings are always constant and used by reference, so we only store the
// pointer on the stack.
self.ptr_map.insert(*ptr, Storage::Stack(stack_base));
stack_base += 1;
stack_base += size_bytes_round_up_to_word_alignment!(n)
}
Type::Array(_) | Type::Struct(_) | Type::Union(_) => {
// Store this aggregate at the current stack base.
Expand Down Expand Up @@ -2123,7 +2123,7 @@ pub fn ir_type_size_in_bytes(context: &Context, ty: &Type) -> u64 {
match ty {
Type::Unit | Type::Bool | Type::Uint(_) => 8,
Type::B256 => 32,
Type::String(_) => 8,
Type::String(n) => size_bytes_round_up_to_word_alignment!(n),
Type::Array(aggregate) => {
if let AggregateContent::ArrayType(el_ty, cnt) = &context.aggregates[aggregate.0] {
cnt * ir_type_size_in_bytes(context, el_ty)
Expand Down
24 changes: 12 additions & 12 deletions sway-core/tests/ir_to_asm/enum_struct_string.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,38 @@ DATA_SECTION_OFFSET[32..64]
lw $ds $is 1
add $$ds $$ds $is
move $r4 $sp ; save locals base register
cfei i32 ; allocate 32 bytes for all locals
cfei i48 ; allocate 48 bytes for all locals
move $r3 $sp ; save register for temporary stack value
cfei i40 ; allocate 40 bytes for temporary struct
cfei i56 ; allocate 56 bytes for temporary struct
lw $r0 data_0 ; literal instantiation
sw $r3 $r0 i0 ; insert_value @ 0
move $r2 $sp ; save register for temporary stack value
cfei i16 ; allocate 16 bytes for temporary struct
cfei i32 ; allocate 32 bytes for temporary struct
lw $r1 data_1 ; literal instantiation
addi $r0 $r2 i0 ; get struct field(s) 0 offset
mcpi $r0 $r1 i8 ; store struct field value
mcpi $r0 $r1 i24 ; store struct field value
lw $r0 data_2 ; literal instantiation
sw $r2 $r0 i1 ; insert_value @ 1
sw $r2 $r0 i3 ; insert_value @ 1
move $r1 $sp ; save register for temporary stack value
cfei i32 ; allocate 32 bytes for temporary struct
cfei i48 ; allocate 48 bytes for temporary struct
addi $r0 $r1 i0 ; get struct field(s) 0 offset
mcpi $r0 $r2 i16 ; store struct field value
mcpi $r0 $r2 i32 ; store struct field value
lw $r0 data_3 ; literal instantiation
sw $r1 $r0 i2 ; insert_value @ 1
sw $r1 $r0 i4 ; insert_value @ 1
lw $r0 data_4 ; literal instantiation
sw $r1 $r0 i3 ; insert_value @ 2
sw $r1 $r0 i5 ; insert_value @ 2
addi $r0 $r3 i8 ; get struct field(s) 1 offset
mcpi $r0 $r1 i32 ; store struct field value
mcpi $r0 $r1 i48 ; store struct field value
lw $r1 $r3 i0 ; extract_value @ 0
lw $r0 data_0 ; literal instantiation
eq $r0 $r1 $r0
jnei $r0 $one i41
addi $r1 $r3 i8 ; extract address
addi $r0 $r4 i0 ; get_ptr
addi $r0 $r4 i0 ; get store offset
mcpi $r0 $r1 i32 ; store value
mcpi $r0 $r1 i48 ; store value
addi $r0 $r4 i0 ; get_ptr
lw $r0 $r0 i2 ; extract_value @ 1
lw $r0 $r0 i4 ; extract_value @ 1
ji i42
lw $r0 data_0 ; literal instantiation
ret $r0
Expand Down
3 changes: 3 additions & 0 deletions sway-ir/src/irtype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ pub enum Type {
impl Type {
/// Return whether this is a 'copy' type, one whose value will always fit in a register.
pub fn is_copy_type(&self) -> bool {
if let Type::String(n) = self {
return *n <= 8;
}
matches!(self, Type::Unit | Type::Bool | Type::Uint(_))
}

Expand Down
2 changes: 1 addition & 1 deletion test/src/e2e_vm_tests/test_programs/should_pass/language/is_reference_type/src/main.sw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn main() -> bool {
assert(!is_reference_type::<byte>());
assert(!is_reference_type::<u64>());

assert(is_reference_type::<str[1]>());
assert(!is_reference_type::<str[1]>());
Copy link
Contributor Author

@mohammadfawaz mohammadfawaz Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we go with this, then we should probably make it clear, in the language, that types that "fit in a word" are always copy types. That includes things like str[1], struct { u64 }, struct { struct { u64 } }, array[1], and so on. It is a bit weird, but I think this would make the specs a bit simpler where the optimization that we've been talking about is actually baked in the language and has to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right. We'd absolutely need regression tests to enforce it for all our types too.

assert(is_reference_type::<b256>());
assert(is_reference_type::<S>());
assert(is_reference_type::<E>());
Expand Down