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

Temporarily disallow asm blocks from writing an initialized registers #3155

Closed
mohammadfawaz opened this issue Oct 26, 2022 · 2 comments · Fixed by #3239
Closed

Temporarily disallow asm blocks from writing an initialized registers #3155

mohammadfawaz opened this issue Oct 26, 2022 · 2 comments · Fixed by #3239
Assignees
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser P: critical Should be looked at before anything else

Comments

@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented Oct 26, 2022

This is due to the addition of mem2reg in #3128. Some odd patterns arise when you allow asm blocks to read and write the same value in IR, which causes mem2reg not to behave well. In any case, we should disallow these patterns temporarily until #2819 is resolved and we have a better mental model regarding asm blocks in IR. write an initialized register.

I think this change can be done in the parser or type checking. Example use cases:

Reading and writing the same register in the same instruction
Writing register r1 even though r1 is initialized to ptr.

    asm(r1: ptr) {
        lw r1 r1 i0;
        r1: u64
    }

Reading and writing the same register in different instructions
Writing registers r1 and r2 even though both are initialized.

    asm(r1: ptr1, r2: reg2) {
        lw r2 r1 i0;
        lw r1 r2 i0;
        r1: u64
    }
@mohammadfawaz mohammadfawaz added P: critical Should be looked at before anything else compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser labels Oct 26, 2022
@otrho
Copy link
Contributor

otrho commented Oct 26, 2022

Checks like these could be permanent IMO, at least in the form of warnings.

@mohammadfawaz mohammadfawaz self-assigned this Oct 28, 2022
@mohammadfawaz
Copy link
Contributor Author

mohammadfawaz commented Oct 30, 2022

I spoke the @vaivaswatha, and the real requirement here is that a register cannot be both initialized in the args list of the asm block AND be assigned to in the body of the block. So my examples above are correct, but my description was not accurate and so I just updated it.

@mohammadfawaz mohammadfawaz changed the title Temporarily disallow asm blocks from reading and writing to the same register Temporarily disallow asm blocks from writing an initialized registers Oct 30, 2022
mohammadfawaz added a commit that referenced this issue Nov 2, 2022
Closes #3155

This is needed due to `mem2reg` and some unclear semantics around
initialized registers being passed as ref/mut ref. Besides, it's a good
idea to restrict `asm` blocks as much as possible to avoid weird
unwanted behavior.

<img width="883" alt="image"
src="https://user-images.githubusercontent.com/59666792/199332120-974d1821-680e-43a1-922d-1a32d491b3cd.png">

* I also removed the `disallow_opcodes` check because the parser takes
care of that now. The parser only allows legal opcodes in `asm` blocks.
* Finally, I update the parser to accept the correct storage opcodes
with `fuel-core 0.13` and updates all the tests.

Co-authored-by: Vaivaswatha N <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser P: critical Should be looked at before anything else
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants