Stream: git-wasmtime

Topic: wasmtime / PR #4540 [cranelift] Rejigger the `compile` API

Wasmtime GitHub notifications bot (Aug 01 2022 at 11:04):

bnjbvr edited PR #4540 from move-emit-to-memory-to-mach-compile-result to main:

This small refactoring makes it clearer to me that emitting to memory
doesn't require anything else from the compilation Context. While it's
a trivial change, it's a small public API change that shouldn't cause
too much trouble, and doesn't seem RFC-worthy. Happy to hear different
opinions about this, though!

Wasmtime GitHub notifications bot (Aug 01 2022 at 17:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 17:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 17:38):

cfallin created PR review comment:

This is the last bit of the refactor I had in mind I think: could we turn this emit_to_memory into just an accessor on self.buffer.data(), and let the embedder do the unsafe dump-the-data-into-a-raw-pointer if they desire?

The major thing we get out of that is to remove the one remaining unsafe API from cranelift-codegen; and it feels more idiomatic to me now that we have a type CompiledCode that wraps up the result to have an accessor providing access to a part of it, rather than a method that splats that content somewhere (noun rather than verb basically). Does that make sense?

Wasmtime GitHub notifications bot (Aug 02 2022 at 12:33):

bnjbvr updated PR #4540 from move-emit-to-memory-to-mach-compile-result to main.

Wasmtime GitHub notifications bot (Aug 02 2022 at 12:34):

bnjbvr submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 12:34):

bnjbvr created PR review comment:

Nice, it even leads to more beautiful call sites which sometimes don't even have to use unsafe :+1:

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:02):

bnjbvr requested cfallin for a review on PR #4540.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:13):

bjorn3 created PR review comment:

Why is this made private?

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:13):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:14):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:16):

bnjbvr created PR review comment:

Is there such an instance of an out-of-tree backend? Thought a getter would be sufficient, as suggested previously.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:16):

bnjbvr submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:16):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:17):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:17):

bjorn3 created PR review comment:

Not that I know of. It might exist in the future. Also there may be benefits to moving all existing backends to a separate crate to make compilation of cranelift itself faster by allowing more parallism.

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:53):

bnjbvr created PR review comment:

Then it makes sense to add some kind of setter whenever it is required. (Good point about moving Cranelift backends to other crates, though; that would certainly help compiling it faster.)

Wasmtime GitHub notifications bot (Aug 02 2022 at 13:53):

bnjbvr submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 19:05):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2022 at 19:05):

cfallin merged PR #4540.

Last updated: Oct 23 2024 at 20:03 UTC