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.

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

Update the spelling of the return register and the return length in the compiler for asm blocks #848

Merged
merged 2 commits into from
Feb 27, 2022

Conversation

mohammadfawaz
Copy link
Contributor

@mohammadfawaz mohammadfawaz commented Feb 26, 2022

Simply changing rv to ret and rl to retl as per the specs

This:

script;

pub fn return_value() -> u64 {
    asm() {
       ret
    }
}

pub fn return_length() -> u64 {
    asm() {
       retl
    }
}

fn main() {
    let x = return_value();
    let y = return_length();
}

now compiles to:

.program:
ji   i4
noop
DATA_SECTION_OFFSET[0..32]
DATA_SECTION_OFFSET[32..64]
lw   $ds $is 1
add  $$ds $$ds $is
move $r0 $ret                 ; return value from inline asm
move $r0 $retl                ; return value from inline asm
ret  $zero                    ; fn main returns unit
.data:

as expected.

@mohammadfawaz
Copy link
Contributor Author

mohammadfawaz commented Feb 26, 2022

@nfurfaro should I add tests for these in the sway/test directory, or will your tests in FuelLabs/sway-lib-std#61 have enough coverage?

@mohammadfawaz
Copy link
Contributor Author

mohammadfawaz commented Feb 26, 2022

Also I noticed that, unless the asm block is exercised from main(), syntax errors do not matter. I assume this is due to the fact that we only check asm blocks in codegen.
Looks like @otrho already has an issue on this: #801

@mohammadfawaz mohammadfawaz self-assigned this Feb 26, 2022
@nfurfaro
Copy link
Contributor

@mohammadfawaz I think if the other reserved registers are tested in the sway repo we should test these there as well, butmy guess is they're not. I will definitely test the wrapper functions I'm adding which use ret and retl.

nfurfaro
nfurfaro previously approved these changes Feb 27, 2022
Copy link
Contributor

@nfurfaro nfurfaro 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.

Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

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

LG.

@mohammadfawaz mohammadfawaz merged commit 0381fc1 into FuelLabs:master Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants