wasmtime / PR #2003 machinst x64: Only use the feature fl... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2003 machinst x64: Only use the feature fl...

Wasmtime GitHub notifications bot (Jul 09 2020 at 15:35):

bnjbvr opened PR #2003 from x64-remove-use-new-backend to main:

Before this patch, running the x64 new backend would require both
compiling with --features experimental_x64 and running with
use_new_backend.

This patches changes this behavior so that the runtime flag is not
needed anymore: using the feature flag will enforce usage of the new
backend everywhere, making using and testing it much simpler:
cargo run --features experimental_x64 ;; other CLI options/flags
This also gives a hint at what the meta language generation would look
like after switching to the new backend.

Compiling only with the x64 codegen flag gives a nice compile time speedup.

As a bonus, this makes it possible to run all the Cranelift and Wasmtime tests just by enabling the feature flag, which should avoid us a few headaches with the Cranelift testing framework in particular :-)

Wasmtime GitHub notifications bot (Jul 09 2020 at 15:35):

bnjbvr requested abrown and jlb6740 for a review on PR #2003.

Wasmtime GitHub notifications bot (Jul 09 2020 at 15:35):

bnjbvr requested abrown and jlb6740 for a review on PR #2003.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:27):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:27):

abrown created PR Review Comment:

Shorter is: let result = if let Some(... and do the if let Err(err) ... check afterwards.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:34):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:34):

abrown created PR Review Comment:

Not necessarily in this PR (but you could if you wanted to): what do you think of using a trait to describe that things like these transform groups can be generated? Then the awkward (isas, new_backend_isas) could be done away with in build.rs. I think that type of thing makes the code easier to use, even if eventually we get rid of the old/new backend distinction.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:37):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:37):

abrown created PR Review Comment:

(You still need an enum like the following but that still seems better than the extra logic above:)
enum BackendType {
  New(TransformGroups),
  Old(TransfomGroups),
}
impl Generator for BackendType ...

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:37):

abrown edited PR Review Comment.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:42):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:42):

abrown created PR Review Comment:

Not sure I'm tracking what this change is about...

Wasmtime GitHub notifications bot (Jul 09 2020 at 23:44):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 08:42):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 08:42):

bnjbvr created PR Review Comment:

Yeah, this fixes a recent regression on the x64 backend, after @yurydelendik switched linking in wasmtime. I think that the underlying object implementation doesn't recognize x86 rel32 relocations in x64 and marks them as generic. The longer term fix for wasmtime is to do what Cranelift old-backend x86 did: pessimize all direct calls (since we don't know the offset to the callee and can't ensure it'll fit in a 32-bits payload, for very large programs) and generate a load function address in register + call indirect. Which also explains the todo below.

Wasmtime GitHub notifications bot (Jul 10 2020 at 13:44):

bnjbvr updated PR #2003 from x64-remove-use-new-backend to main:

Before this patch, running the x64 new backend would require both
compiling with --features experimental_x64 and running with
use_new_backend.

This patches changes this behavior so that the runtime flag is not
needed anymore: using the feature flag will enforce usage of the new
backend everywhere, making using and testing it much simpler:
cargo run --features experimental_x64 ;; other CLI options/flags
This also gives a hint at what the meta language generation would look
like after switching to the new backend.

Compiling only with the x64 codegen flag gives a nice compile time speedup.

As a bonus, this makes it possible to run all the Cranelift and Wasmtime tests just by enabling the feature flag, which should avoid us a few headaches with the Cranelift testing framework in particular :-)

Wasmtime GitHub notifications bot (Jul 10 2020 at 13:45):

bnjbvr requested abrown and jlb6740 for a review on PR #2003.

Wasmtime GitHub notifications bot (Jul 10 2020 at 15:11):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 15:11):

abrown created PR Review Comment:

Looks like we still need &mut fmt: https://github.com/bytecodealliance/wasmtime/pull/2003/checks?check_run_id=858163032#step:11:115

Wasmtime GitHub notifications bot (Jul 10 2020 at 17:00):

bnjbvr updated PR #2003 from x64-remove-use-new-backend to main:

Before this patch, running the x64 new backend would require both
compiling with --features experimental_x64 and running with
use_new_backend.

This patches changes this behavior so that the runtime flag is not
needed anymore: using the feature flag will enforce usage of the new
backend everywhere, making using and testing it much simpler:
cargo run --features experimental_x64 ;; other CLI options/flags
This also gives a hint at what the meta language generation would look
like after switching to the new backend.

Compiling only with the x64 codegen flag gives a nice compile time speedup.

As a bonus, this makes it possible to run all the Cranelift and Wasmtime tests just by enabling the feature flag, which should avoid us a few headaches with the Cranelift testing framework in particular :-)

Wasmtime GitHub notifications bot (Jul 15 2020 at 11:11):

bnjbvr merged PR #2003.

Last updated: Nov 22 2024 at 16:03 UTC