Stream: git-wasmtime

Topic: wasmtime / PR #8515 wasmtime: Use ConstExpr for data segm...


view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 08:34):

jameysharp opened PR #8515 from jameysharp:data-const-expr to bytecodealliance:main:

This shouldn't change any behavior currently, but prepares us for supporting extended constant expressions.

I want to think about this a bit more before merging it but here's a draft for @fitzgen or anyone else who wants to take a look.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 08:36):

jameysharp edited PR #8515:

This shouldn't change any behavior currently, but prepares us for supporting extended constant expressions.

I want to think about this a bit more before merging it but here's a draft for @fitzgen or anyone else who wants to take a look. This is basically the same change as #8514 but for memories instead of tables, and the existing code was more complicated for this case.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 18:29):

jameysharp updated PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 18:37):

jameysharp updated PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 18:56):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 18:56):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 18:56):

fitzgen created PR review comment:

nitpicky suggestion, take it or leave it: I'd personally rename this InitMemoryAtRunTime and the other impl as InitMemoryAtCompileTime. I think it is a little more clear but maybe it is just me.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 18:56):

fitzgen created PR review comment:

It might be better to turn ConstEvalContext into a trait -- I initially did that but then every use site was the same so I made it concrete. But if it were a trait, then we could use a Result for getting globals and funcrefs and return an Err when they aren't available at compile time. This would allow both reusing eval here so you don't have to do this matching (nor in the other PR, iirc) and also support const expression offsets like (i32.add (i32.const 3) (i32.const 5)) for compile-time heap image computation.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 20:51):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2024 at 20:51):

jameysharp created PR review comment:

I wasn't entirely happy with the names I picked and considered doing something like you suggest. So it's not hard to convince me on this one!

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 20:28):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 20:28):

jameysharp created PR review comment:

It turns out it's easy to turn ConstEvalContext into a trait, but it's not useful without bigger changes that I haven't been able to figure out yet.

ConstExprEvaluator can only be used within the wasmtime crate, because it relies on ValRaw. Everything that does const evaluation in that crate already has access to an Instance. All the places that need const evaluation before instantiation are in wasmtime-environ or wasmtime-types.

I tried factoring out a type-specific, integer-only const-evaluator that can live in wasmtime-types alongside ConstExpr and ConstOp. But the general-purpose const-evaluator is still necessary for initializing globals and I'm not real happy with duplicating this functionality.

Maybe we need a trait in wasmtime-types that Val or ValRaw can implement?

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 21:41):

jameysharp updated PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 21:44):

jameysharp updated PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 21:44):

jameysharp has marked PR #8515 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 21:44):

jameysharp requested elliottt for a review on PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 21:44):

jameysharp requested wasmtime-core-reviewers for a review on PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 21:49):

jameysharp commented on PR #8515:

I've decided it's more complicated than I want to tackle in this PR to make const-eval available at compile time, so I hope to come back to that later. I've renamed types as @fitzgen suggested (though I decided I liked AtInstantiation better than AtRunTime).

And I've spent a couple days thinking about this and decided I don't have particularly better ideas for anything in here, so I've marked it ready for review now.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 22:43):

jameysharp edited PR #8515:

This shouldn't change any behavior currently, but prepares us for supporting extended constant expressions.

~I want to think about this a bit more before merging it but here's a draft for @fitzgen or anyone else who wants to take a look.~ This is ready for review. This is basically the same change as #8514 but for memories instead of tables, and the existing code was more complicated for this case.

view this post on Zulip Wasmtime GitHub notifications bot (May 03 2024 at 23:02):

elliottt requested fitzgen for a review on PR #8515.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2024 at 15:56):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2024 at 15:56):

fitzgen created PR review comment:

Maybe we need a trait in wasmtime-types that Val or ValRaw can implement?

Yeah I think we'll have to do something roughly like this.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2024 at 16:01):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2024 at 16:24):

fitzgen merged PR #8515.


Last updated: Dec 23 2024 at 12:05 UTC