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

Expose wasm instrinsics for grow_memory and current_memory instructions #23

Closed
fitzgen opened this issue Jan 17, 2018 · 28 comments
Closed

Comments

@fitzgen
Copy link
Member

fitzgen commented Jan 17, 2018

I've wanted to use these instructions, but no way to access it, and can't write WAT by hand either because of #22

@pepyakin
Copy link
Member

Actually you can access some intrinsics, see for example

@fitzgen
Copy link
Member Author

fitzgen commented Jan 17, 2018

Will that #[link_name] always work? Is it stable in the Rust sense? Those comments mention how llvm doesn't currently expose the return value from grow_memory but eventually will -- will my code break when that transition happens?

@alexcrichton
Copy link
Contributor

@fitzgen nah that's an unstable (permanently) feature, we'd want to look to exposing these intrinsics elsewhere rather than using the example there. That's just what's needed to implement it in libstd today.

@koute
Copy link

koute commented Jan 17, 2018

If you want to use it on stable Rust right now and be sure your code won't break in the future you could add an extern { fn grow_memory(pages: u32); } in your code, and then postprocess the .wasm file and replace that import with an actual function which would call the grow_memory instruction for you.

Of course having this actually exposed by Rust as an intrinsic would be a lot better.

@fitzgen
Copy link
Member Author

fitzgen commented Jan 17, 2018

Yeah. Or if we had a working linker, I could define out-of-line functions in WAT, compile them to .wasm in build.rs (or just check in the .wasm to the crate repo), and then link against that .wasm file.

Neither seems ideal, although linking doesn't seem too terrible.

I think we should have proper intrinsics, though. How would we go about adding and stabilizing them?

@pepyakin
Copy link
Member

pepyakin commented Jan 17, 2018

I bet that this is far away from stable, instrinsics could get new immediate(s) and I think there is a high chance that the code will break after the transition.

(oops, seems im loo late)

@sunfishcode
Copy link

FYI, grow_memory and current_memory are likely to be renamed in the wasm spec to mem.grow and mem.size; see WebAssembly/spec#627.

I'm expecting to introduce new intrinsics in LLVM (and new builtins in clang) with the new names soon (and they'll probably have an operand for the wasm immediate too).

@alexcrichton
Copy link
Contributor

Thanks for the heads up @sunfishcode!

My thinking for how we'd expose this in Rust is probably to add it to the stdsimd crate. That crate's moreso "vendor intrinsics" rather than just SIMD stuff. Currently it's not integrated into libcore but hopefully that'll happen in the somewhat near future!

@sunfishcode
Copy link

The wasm CG is moving forward with the rename, though the decision is not fully final yet.

I've tentatively added intrinsics with the proposed names in LLVM, along with immediate operands, here.

@alexcrichton
Copy link
Contributor

@sunfishcode so right now libstd in Rust isn't great about exposing vendor-specific (platform-specific) intrinsics like SIMD on x86, but that's hopefully changing soon! For things on x86 we've been using Intel's intrinsic guide as a reference point for what the signatures should look like in Rust. In essence we're hoping there's vendor definitions of what we should be doing so there's an obvious path to stabilization for us.

With that in mind, is there something similar for wasm? In other words, is there an 'official definition' of sorts for how these intrinsics should be exposed with a C-like signature? If not no worries, we can always tweak it as it's a nightly target anyway :)

@sunfishcode
Copy link

In clang, they now look like this:

    size_t __builtin_wasm_mem_size(int32_t imm);
    size_t __builtin_wasm_mem_grow(int32_t imm, size_t delta);

where the imm argument is required to be a compile-time constant.

There's no official documentation for these yet, but I expect that these are what will eventually be documented. Provided that the CG doesn't reverse course on the rename.

@alexcrichton
Copy link
Contributor

Ok thanks!

We unfortunately don't have a great way of specifying "this argument must be constant" in Rust right now...

@Diggsey
Copy link

Diggsey commented Feb 1, 2018

You could use an intrinsic with an attribute on I guess, but really you want const generics.

@fitzgen
Copy link
Member Author

fitzgen commented Feb 1, 2018

@sunfishcode, why do these return size_t? The copy of the standard I've printed out says that these intrinsics return an i32 and use -1 to signal an error. How would I check for an error with the size_t signatures? Additionally, grow_memory expects an i32 delta, which implies to me that one could use negative numbers to shrink memory and return space to the engine.

the imm argument is required to be a compile-time constant.

This seems unfortunate. Are we supposed to call this intrinsic in a loop until we have grown memory to its desired dynamic size? What is the motivation for this restriction?

@pepyakin
Copy link
Member

pepyakin commented Feb 1, 2018

@fitzgen isn't size_t is always 32-bit integer in wasm32?

And yes, you can't shrink memory with grow_memory. In fact, you can't shrink memory at all

I guess, imm is constant time expression is beacuse it is supposed to be used for things like memory index, etc

@fitzgen
Copy link
Member Author

fitzgen commented Feb 1, 2018

Oh, woops I misunderstood the imm parameter -- never mind about that!

@fitzgen isn't size_t is always 32-bit integer in wasm32?

size_t is unsigned, though, right?

Why not use int32_t?

@sunfishcode
Copy link

The imm argument corresponds to an immediate field at the wasm level. It's not possible to encode the instruction without knowing the value.

size_t is unsigned, and that's how mem.grow interprets its argument. It is deliberately designed to be unable to shrink memory.

@pepyakin
Copy link
Member

pepyakin commented Feb 1, 2018

Ah, I see, i misunderstood you too : )

I believe we can just check against 0xFFFFFFFF?

@fitzgen
Copy link
Member Author

fitzgen commented Feb 1, 2018

size_t is unsigned, and that's how mem.grow interprets its argument. It is deliberately designed to be unable to shrink memory.

What about the return value?

I believe we can just check against 0xFFFFFFFF?

This will work, but it really seems less elegant than accurately expressing that -1 can be returned in the type, by using a signed integer.

@sunfishcode
Copy link

Yeah, that's a good point about the return value. I think it's meant to conceptually be something like Result<usize, ()>, except that wasm currently has no simple way to implement that, so it uses a sentinel value instead. The value in the non-failure case is still conceptually unsigned. So I can see arguments both ways.

@fitzgen
Copy link
Member Author

fitzgen commented Feb 1, 2018

I guess we are getting into the weeds here.

I understand not wanting to cut in half the range of the success values (since they become negative with a signed integer), so maybe the spec should change to use u32 and explicitly call out 0xffffffff as a sentinel.

@sunfishcode
Copy link

You can write usize::MAX if you don't want to write 0xffffffff. But I'd prefer to keep i/u-size rather than switch to i/u-32 because if wasm64 happens, these will all become 64-bit integers there, and we'd want to write code that works on both.

@fitzgen
Copy link
Member Author

fitzgen commented Feb 1, 2018

@alexcrichton are the #[link_name = "llvm.wasm.current.memory.i32"] names going to change?

@alexcrichton
Copy link
Contributor

@fitzgen perhaps yeah but that's just an implementation for an eventual stable version of these intrinsics, I don't think we'll ever expose that implementation detail on stable.

@sunfishcode
Copy link

At this time, the WebAssembly CG is still considering the rename, and the new names are not yet final.

@alexcrichton
Copy link
Contributor

Ok thanks for the update @sunfishcode! In the meantime I've taken the current instruction names and proposed adding them to stdsimd, the current house for platform-specific intrinsics -- rust-lang/stdarch#361

@est31
Copy link

est31 commented Mar 9, 2018

While stdsimd seems okay for memory intrinsics, I wonder whether it is still a good place for possible future DOM or GC intrinsics. Because those might not be provided in the non-web use case.

@alexcrichton
Copy link
Contributor

I'm gonna close this as these will soon (hopefully) be merged into rust-lang/rust under std::arch::wasm32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants