Intrinsics for the VM's storage opcodes. #2508

vaivaswatha · 2022-08-10T14:21:45Z

This PR provides the following intrinsics:

__state_load_word (key: b256) -> u64;
__state_store_word (key: b256, value: u64);
__state_load_quad (key: b256, addr: u64);
__state_store_quad (key: b256, addr: u64);

All of them compile to equivalent IR instructions. The last two (quad versions) required some changes to asm generation since they use int_to_ptr as inputs to the IR instructions for quad storage.

While adding the tests, a flaw I discovered was that: If the intrinsics are called from a script, passing an address that belongs to the script, we hit a runtime error (because the address is now referenced in the contract, not the calling script). This indicates that we'll need some static analysis around passing addresses - flagging such issues. Another issue would be if we take the address of a const, if at all that is allowed - I haven't checked. These are issues with the __addr_of intrinsic rather than this one, so I suppose I can file a separate Issue on that?

Closes #2514

mohammadfawaz · 2022-08-10T14:37:36Z

a flaw I discovered was that: If the intrinsics are called from a script

We should certainly error out at compile-time if storage intrinsics are used in a script!

vaivaswatha · 2022-08-10T14:59:44Z

a flaw I discovered was that: If the intrinsics are called from a script

We should certainly error out at compile-time if storage intrinsics are used in a script!

I think that already happens. The problem is if you take a local variable's address in a script, then call a (wrapper) function in the contract that in-turn calls one of these intrinsics using that address, then we have a runtime error.

mohammadfawaz · 2022-08-10T16:28:46Z

This is great! Thanks Vaivas :)

Three small requests:

Do you mind creating a short issue that explains the motivation behind this change and link the PR to it? It's probably good to mention that this is one task towards implementing the intrinsics RFC that you proposed.
Will you also describe the source of the runtime error in another issue so that we don't forget about it?
Finally, can we now use the intrinsic in the standard library to replace asm blocks? Is it a good idea to this in this PR?

vaivaswatha · 2022-08-10T16:31:46Z

This is great! Thanks Vaivas :)

Three small requests:

* Do you mind creating a short issue that explains the motivation behind this change and link the PR to it? It's probably good to mention that this is one task towards implementing the intrinsics RFC that you proposed.

* Will you also describe the source of the runtime error in another issue so that we don't forget about it?

* Finally, can we now use the intrinsic in the standard library to replace `asm` blocks? Is it a good idea to this in this PR?

I meant to do the 3rd one in this PR and forgot all about it 🤦‍♂️ . I'll do all three of these and update back here. Thanks Mo.

vaivaswatha · 2022-08-11T13:40:40Z

This is great! Thanks Vaivas :)

Three small requests:

Do you mind creating a short issue that explains the motivation behind this change and link the PR to it? It's probably good to mention that this is one task towards implementing the intrinsics RFC that you proposed.

Will you also describe the source of the runtime error in another issue so that we don't forget about it?

Finally, can we now use the intrinsic in the standard library to replace asm blocks? Is it a good idea to this in this PR?

New issues: #2515 and #2514

sezna · 2022-08-11T14:46:09Z

One thing I am curious about is the portability of these intrinsics. Given that the EVM's word size is different, the quad word intrinsic would have to load 256 * 4 == 1024 bits in order for the name to make sense. Is there a naming scheme that could give us more freedom to implement these optimally? Perhaps state_load_256 or something?

mohammadfawaz · 2022-08-11T14:47:12Z

One thing I am curious about is the portability of these intrinsics. Given that the EVM's word size is different, the quad word intrinsic would have to load 256 * 4 == 1024 bits in order for the name to make sense. Is there a naming scheme that could give us more freedom to implement these optimally? Perhaps state_load_256 or something?

+1 for this.

otrho · 2022-08-12T01:17:47Z

Aren't we going to have different intrinsics for different targets? So maybe since these are Fuel specific they should be __fuelvm_state_load_word() and we can have __evm_state_load_word() eventually too. And, then I guess different instructions in the IR... 🤔

I was 🤔 about this the other day actually -- platform specific stuff in the IR should probably be generalised with wrappers. We're going to have to do this with ASM blocks which are currently FuelVM specific in the IR, and so any platform specific stuff will need the same. Storage counts here -- it's very, very similar between FuelVM and EVM but not the same.

sezna · 2022-08-12T03:58:50Z

Aren't we going to have different intrinsics for different targets?

Hmm, my mental model has been that we will use the same intrinsics and accomplish portability by implementing the codegen differently. Otherwise we aren't increasing portability with intrinsics relative to asm blocks, since you'd have to rewrite both of them. Platform specific stuff can live in asm perhaps?

How do portable intrinsics work in e.g. rust? I guess they're platform specific too, thinking about things like the AVX intrinsics....maybe there's a middle ground where some are disallowed for certain backends, like the ones related to the fuel tx frame -- those would need to be disallowed for EVM..

Just having a stream of consciousness here...

otrho · 2022-08-12T04:20:09Z

I was thinking yeah, a certain subset of intrinsics will just not compile, with an error, if your target isn't right for them. Some intrinsics simply won't exist for certain targets, especially if we wrap basic ASM ops (in the ongoing effort to remove ASM blocks from the libs in the name of type safety).

ASM blocks themselves are going to have to change drastically, though this isn't really the place for that discussion. But they're going to have to be more opaque than they are now, and maybe intrinsics should be the same.

We could actually have just one 'intrinsic' which is parameterised with an enum. So __size_of<T>() becomes __builtin<T>(SizeOf), or we have e.g., __builtin<T>(Eq, lhs: T, rhs: T) -> bool and __builtin is very much special cased and parameterised based on target. It would have to magically allow any number of type parameters and args and return type.

I'm not sure if this is less work and/or easier to implement though.

sezna · 2022-08-12T04:23:13Z

We also need to standardize how we define what intrinsics are allowable for what backends, and it should be thoroughly considered..

We could actually have just one 'intrinsic' which is parameterised with an enum. So __size_of() becomes __builtin(SizeOf), or we have e.g., __builtin(Eq, lhs: T, rhs: T) and __builtin is very much special cased and parameterised based on target. It would have to magically allow any number of type parameters and args.

What would be the benefit of restructuring in this way?

otrho · 2022-08-12T04:24:35Z

There is one intrinsic whose behaviour is entirely data driven based on the target.

You could add or modify targets without having to change the actual compiler core/type checker.

vaivaswatha · 2022-08-12T06:30:35Z

I've now updated storage.sw to use the new intrinsics. To workaround type limitations on the arguments and return type of the intrinsics, I'm using asm to do type casts (this isn't a problem at runtime because of the is_reference_type guard). Ideally, this should be solved with type constraints and not using is_reference_type as detailed here.

So that leaves only the question of names for these intrinsics (the discussion going on above related to different targets). I'll update the names once that is finalized, but the rest of the code can be reviewed now I think.

otrho

Comments, below, all good otherwise.

otrho · 2022-08-12T06:38:39Z

sway-core/src/asm_generation/from_ir.rs

+        let val_reg = if matches!(
+            &self.context.values[val.0].value,
+            ValueDatum::Instruction(Instruction::IntToPtr(..))


Rather than special case IntToPtr here would it work instead to update resolve_ptr() to handle both GetPointer and IntToPtr?

Rather than special case IntToPtr here would it work instead to update resolve_ptr() to handle both GetPointer and IntToPtr?

I considered that, but there's some computation that's done later which isn't needed for IntToPtr (but needed if resolved as a stack pointer in resolve_ptr. So it wouldn't serve any purpose since I'll need to handle them differently here anyway.

otrho · 2022-08-12T06:44:27Z

sway-core/tests/ir_to_asm/storage_store_load_intrinsics.ir

@@ -0,0 +1,486 @@
+contract {
+    fn main<ea1a0f91>() -> u64, !3 {


As per #2505 we really need to move these tests away from here and use FileCheck. This is a bit ridiculous. 😃

otrho · 2022-08-12T06:50:19Z

sway-lib-std/src/storage.sw

+        let loaded_word = __state_load_word(key);
+        asm (l: loaded_word) {
+            l: T
        }


Can we not turbofish this one here? Was that also part of the discussion around this issue?

Can we not turbofish this one here? Was that also part of the discussion around this issue?

You mean using std::mem::read<T>? That wouldn't work as it ends up actually doing a load, assuming the value to be an address (in case of non-reference types).

The asm block is just casting the loaded word to T -- if it's a copy type then we should be able to use __state_load_word::<T>(key) to get it in the right type from the start, no? I didn't check, is __state_load_word() just returning a u64 right now? Maybe it should return T.

Or is this just the problem we can't solve until we have trait constraints?

It really only returns u64, so having its type to return T would be incorrect. But if it's returning only u64, then when used with is_reference_type<T> in a generic function, we end up with this problem we have. If we could restrict it with trait contraints, then we wouldn't have this call in a non u64 context at all.

Right, OK. I kinda feel like we should only have two intrinsics -- read and write, and whether they need to use SRW/SWW or SRWQ/SWWQ can be decided by the back end. Then the read functions could just return T.

But I'm sure that won't work either for some reason. OK, this'll do for now but I can feel the technical debt growing weekly. 😄

otrho · 2022-08-12T06:55:08Z

sway-lib-std/src/storage.sw

+            asm() {
                cfei i32;
-                srwq v k;
            };


Next is an __alloca() intrinsic to wrap CFEI since it's a bit weird to grab the stack pointer with a call but then allocate some space with an ASM block. ~~I guess it could go in a library function for now...~~

otrho · 2022-08-12T07:42:40Z

sway-lib-std/src/alloc.sw

+pub fn alloca<T>() -> u64 {
+    let current_pointer = stack_ptr();
+    asm() {
+        cfei i32;


This is allocating 32 bytes, where it needs to allocate __size_of<T>().

This is allocating 32 bytes, where it needs to allocate '__size_of()`.

Oops, yes. Let me fix this.

Dropping (and reverting related commits) this as it needs to be an intrinsic. The argument to cfei must be an immediate :-(

This reverts commit cad780b.

This reverts commit 6db548d.

otrho

Alright. Approving and then I'm out of here. Have a good weekend!

vaivaswatha · 2022-08-12T08:09:41Z

Alright. Approving and then I'm out of here. Have a good weekend!

Thank you and you too !

…_intrinsics

vaivaswatha and others added 3 commits August 9, 2022 11:51

Add storage load/store word intrinsics

ecee49b

Storage load/store quad intrinsics

4ab8013

Merge branch 'master' into vaivaswatha/storage_intrinsics

9863a44

vaivaswatha self-assigned this Aug 10, 2022

Update hash of a test

2c0a816

vaivaswatha and others added 6 commits August 10, 2022 20:34

fix clippy

3aee84a

Process key_reg after val_reg to avoid diffs in asm gen tests

26807c7

Merge branch 'master' into vaivaswatha/storage_intrinsics

799b981

Fix a deployment based test with right hash

e7f8168

Add asm gen test

75e7a9c

Merge branch 'master' into vaivaswatha/storage_intrinsics

ca164d3

vaivaswatha marked this pull request as ready for review August 10, 2022 16:22

vaivaswatha requested a review from a team August 10, 2022 16:22

vaivaswatha added 3 commits August 11, 2022 16:35

Check arguments after monomorphization, during IR gen

82a2e6e

Replace asm with intrinsics in storage.sw

88e954d

Restore storage.sw

2c2015f

minor fixes

d1f11e2

Update storage.sw and add exhaustive tests

ba5e609

vaivaswatha requested a review from otrho as a code owner August 12, 2022 06:06

vaivaswatha and others added 2 commits August 12, 2022 11:36

Merge branch 'master' into vaivaswatha/storage_intrinsics

4d433e3

Update JSON ABI

66fe14c

otrho reviewed Aug 12, 2022

View reviewed changes

otrho requested a review from a team August 12, 2022 06:57

vaivaswatha added 2 commits August 12, 2022 13:00

Move cfei into an alloca lib function

cad780b

Update contract hash in calling script

6db548d

otrho reviewed Aug 12, 2022

View reviewed changes

vaivaswatha added 2 commits August 12, 2022 13:24

Revert "Move cfei into an alloca lib function"

a6a59d1

This reverts commit cad780b.

Revert "Update contract hash in calling script"

594a5b9

This reverts commit 6db548d.

otrho previously approved these changes Aug 12, 2022

View reviewed changes

otrho requested a review from a team August 12, 2022 08:04

Merge remote-tracking branch 'origin/master' into vaivaswatha/storage…

fa82947

…_intrinsics

vaivaswatha dismissed otrho’s stale review via fa82947 August 16, 2022 06:09

vaivaswatha requested a review from otrho August 16, 2022 06:27

otrho approved these changes Aug 16, 2022

View reviewed changes

otrho requested a review from a team August 16, 2022 06:59

sezna approved these changes Aug 16, 2022

View reviewed changes

Merge branch 'master' into vaivaswatha/storage_intrinsics

a27f9b3

vaivaswatha enabled auto-merge (squash) August 16, 2022 13:34

Merge branch 'master' into vaivaswatha/storage_intrinsics

80858ab

vaivaswatha merged commit a5d5a26 into master Aug 16, 2022

vaivaswatha deleted the vaivaswatha/storage_intrinsics branch August 16, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intrinsics for the VM's storage opcodes. #2508

Intrinsics for the VM's storage opcodes. #2508

vaivaswatha commented Aug 10, 2022 •

edited

Loading

mohammadfawaz commented Aug 10, 2022

vaivaswatha commented Aug 10, 2022

mohammadfawaz commented Aug 10, 2022 •

edited

Loading

vaivaswatha commented Aug 10, 2022

vaivaswatha commented Aug 11, 2022

sezna commented Aug 11, 2022

mohammadfawaz commented Aug 11, 2022 •

edited

Loading

otrho commented Aug 12, 2022

sezna commented Aug 12, 2022

otrho commented Aug 12, 2022 •

edited

Loading

sezna commented Aug 12, 2022

otrho commented Aug 12, 2022 •

edited

Loading

vaivaswatha commented Aug 12, 2022

otrho left a comment

otrho Aug 12, 2022

vaivaswatha Aug 12, 2022

otrho Aug 12, 2022

otrho Aug 12, 2022

vaivaswatha Aug 12, 2022

otrho Aug 12, 2022

otrho Aug 12, 2022

vaivaswatha Aug 12, 2022

otrho Aug 12, 2022

otrho Aug 12, 2022 •

edited

Loading

otrho Aug 12, 2022 •

edited

Loading

vaivaswatha Aug 12, 2022

vaivaswatha Aug 12, 2022

otrho left a comment

vaivaswatha commented Aug 12, 2022

Intrinsics for the VM's storage opcodes. #2508

Intrinsics for the VM's storage opcodes. #2508

Conversation

vaivaswatha commented Aug 10, 2022 • edited Loading

mohammadfawaz commented Aug 10, 2022

vaivaswatha commented Aug 10, 2022

mohammadfawaz commented Aug 10, 2022 • edited Loading

vaivaswatha commented Aug 10, 2022

vaivaswatha commented Aug 11, 2022

sezna commented Aug 11, 2022

mohammadfawaz commented Aug 11, 2022 • edited Loading

otrho commented Aug 12, 2022

sezna commented Aug 12, 2022

otrho commented Aug 12, 2022 • edited Loading

sezna commented Aug 12, 2022

otrho commented Aug 12, 2022 • edited Loading

vaivaswatha commented Aug 12, 2022

otrho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrho Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

otrho Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrho left a comment

Choose a reason for hiding this comment

vaivaswatha commented Aug 12, 2022

vaivaswatha commented Aug 10, 2022 •

edited

Loading

mohammadfawaz commented Aug 10, 2022 •

edited

Loading

mohammadfawaz commented Aug 11, 2022 •

edited

Loading

otrho commented Aug 12, 2022 •

edited

Loading

otrho commented Aug 12, 2022 •

edited

Loading

otrho Aug 12, 2022 •

edited

Loading

otrho Aug 12, 2022 •

edited

Loading