Stream: git-wasmtime

Topic: wasmtime / PR #4453 cranelift: Use `cranelift-jit` in run...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2022 at 14:30):

afonso360 opened PR #4453 from testrunner-jit to main:

:wave: Hey,

This PR changes our run tests to run via cranelift-jit. Its based on top of #4450 so its probably not worth doing any serious review until that is merged.

Using cranelift-jit in run tests allows us to preform relocations and
libcalls. This is important since some instruction lowerings fallback
to libcall's when an extension is missing, or when it's too complicated
to implement manually.

This is also a first step to being able to test call's between functions
in the runtest suite. It should also make it easier to eventually test
TLS relocations, symbol resolution and ABI's.

Another benefit of this is that we also get to test the JIT more, since
it now runs the runtests, and gets some fuzzing via fuzzgen (which
uses the SingleFunctionCompiler).

This change causes regressions in terms of runtime for the filetests.
I haven't done any serious benchmarking but what I've been seeing is
that it now takes about ~3 seconds to run the testsuite while it
previously took around 2 seconds.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2022 at 14:31):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2022 at 14:31):

afonso360 created PR review comment:

I'm slightly concerned about this change.

msvcrt does not define ceilf/floorf/nearbyintf/truncf, which are some of the LibCall's that we define. I cannot find the dll that defines those functions pre VS2015.

After VS2015 Microsoft created the Universal C Runtime which defines ucrtbase.dll that does contain all of those functions. However that only became standard with Windows 10 in 2015. Previous versions of Windows can have that dll, but only if they install Visual Studio 2015.

We cannot search msvcrt and fallback to ucrtbase since mixing the two runtimes will cause weird behaviour, probably at the worst time possible.

Does anyone know where we can find those functions? Or alternativley what are the minimum requirements that we have for cranelift and can we upgrade to UCRT?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2022 at 14:35):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2022 at 14:35):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2022 at 14:40):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2022 at 08:09):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2022 at 08:19):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2022 at 10:45):

afonso360 has marked PR #4453 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 18:23):

cfallin created PR review comment:

I guess the reason that we can't have e.g. a PhantomData carrying a lifetime 'a representing the trampoline here is that the lifetime is that of module above (i.e. it's be a self-referential struct), is that right? If so let's add a comment here noting that the lifetime corresponds to module and that we don't ever leak the pointers.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 18:23):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 18:23):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 18:23):

cfallin created PR review comment:

Hmm, I don't have too many opinions on what is reasonable as a minimum version requirement. Keeping to a more conservative set of requirements is always good, everything else being equal; but here if that means we'd have to polyfill (and those polyfills would be tested only on older platforms, i.e. rarely or not at all) then I'm not sure what value that carries.

@peterhuene do you have any thoughts on this re: Windows runtime versions?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 20:23):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 20:25):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 20:26):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 20:26):

afonso360 created PR review comment:

We actually don't need these pointers. I switched the implementation so that we hold FuncId's from the JITModule, and query the address when calling the function.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2022 at 20:26):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 20:40):

afonso360 edited PR #4453 from testrunner-jit to main:

:wave: Hey,

This PR changes our run tests to run via cranelift-jit. Its based on top of #4450 so its probably not worth doing any serious review until that is merged.

Using cranelift-jit in run tests allows us to preform relocations and
libcalls. This is important since some instruction lowerings fallback
to libcall's when an extension is missing, or when it's too complicated
to implement manually.

This is also a first step to being able to test call's between functions
in the runtest suite. It should also make it easier to eventually test
TLS relocations, symbol resolution and ABI's.

Another benefit of this is that we also get to test the JIT more, since
it now runs the runtests, and gets some fuzzing via fuzzgen (which
uses the SingleFunctionCompiler).

This change causes regressions in terms of runtime for the filetests.
I haven't done any serious benchmarking but what I've been seeing is
that it now takes about ~3 seconds to run the testsuite while it
previously took around 2 seconds.

Edit:
This also closes #3030 since we can now do relocations in runtests

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 20:44):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 21:15):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 21:15):

jameysharp created PR review comment:

I'm told Windows 8.1 reaches end of life in January, which seems to me like a good enough reason for Cranelift to require Windows 10. But others may disagree.

Since we're looking at using the libm crate for #4517, I thought I'd check if we could get these functions from there, instead of worrying about Windows C runtime versions. It does have ceilf, floorf, and truncf, but unfortunately not nearbyintf, so I'm not sure if that helps with this question.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 07:21):

afonso360 updated PR #4453 from testrunner-jit to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 07:56):

afonso360 created PR review comment:

@cfallin I would like to move forward with this PR since it's blocking some stuff on the fuzzer (getting function calls working).

Would @jameysharp 's reasoning that pretty soon all supported windows versions are going to have ucrt be sufficient? An alternative could be to keep using msvcrt, but we would need to disable some ops on the runtest suite, which is also not ideal.

We can also go with polyfill's as @jameysharp mentioned, I don't like this approach since we would probably keep needing to patch holes in msvcrt. In https://github.com/bjorn3/rustc_codegen_cranelift/pull/1254 we also found that fabsf is missing from msvcrt, so that's going to be another one that we need. With the bonus that we know that one is definitely broken in libm.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 07:56):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 07:56):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 08:26):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 08:27):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 18:32):

cfallin created PR review comment:

Yeah, I think that's fine; Win10 as a minimum requirement here is probably an acceptable compromise, given the context.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2022 at 18:32):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp created PR review comment:

Changing SingleFunctionCompiler::compile from taking &mut self to just self feels unfortunate, since it forces callers to build a new TargetIsa for every function they compile. What would you think of changing JITBuilder to take a shared reference instead? Either &dyn TargetIsa or Rc<dyn TargetIsa>, whichever is easier. In the latter case, SingleFunctionCompiler would also take an Rc instead of a Box.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp created PR review comment:

What is this change for?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp created PR review comment:

I'd prefer not to move this code into the loop, if my earlier suggestion on shared references to the TargetIsa works out.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp created PR review comment:

I'd prefer not to move this code into the loop, if my earlier suggestion on shared references to the TargetIsa works out.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 19:19):

jameysharp created PR review comment:

If it works out to keep the SingleFunctionCompiler reusable, do you think it'd be worth keeping the cache of trampolines around too?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 20:37):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 20:37):

afonso360 created PR review comment:

The name user(0, 0) was the trampoline IIRC and the JIT was complaining about it. I'm not too sure, but I can go check.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 20:40):

afonso360 created PR review comment:

I remember (sorry, this was a while ago) trying to change to &dyn TargetIsa, but it brought a bunch of changes due to lifetimes, and I gave up on that path.

I didn't try Rc<> but see the comment down below.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 20:40):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 09 2022 at 21:54):

jameysharp merged PR #4453.

Last updated: Nov 22 2024 at 16:03 UTC