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 viafuzzgen
(which
uses theSingleFunctionCompiler
).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.
afonso360 submitted PR review.
afonso360 created PR review comment:
I'm slightly concerned about this change.
msvcrt
does not defineceilf
/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 toucrtbase
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?
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 updated PR #4453 from testrunner-jit
to main
.
afonso360 updated PR #4453 from testrunner-jit
to main
.
afonso360 updated PR #4453 from testrunner-jit
to main
.
afonso360 has marked PR #4453 as ready for review.
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 ofmodule
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 tomodule
and that we don't ever leak the pointers.
cfallin submitted PR review.
cfallin submitted PR review.
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?
afonso360 updated PR #4453 from testrunner-jit
to main
.
afonso360 updated PR #4453 from testrunner-jit
to main
.
afonso360 submitted PR review.
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.
afonso360 updated PR #4453 from testrunner-jit
to main
.
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 viafuzzgen
(which
uses theSingleFunctionCompiler
).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
afonso360 updated PR #4453 from testrunner-jit
to main
.
jameysharp submitted PR review.
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
, andtruncf
, but unfortunately notnearbyintf
, so I'm not sure if that helps with this question.
afonso360 updated PR #4453 from testrunner-jit
to main
.
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 usingmsvcrt
, 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.
afonso360 submitted PR review.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
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.
cfallin submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
Changing
SingleFunctionCompiler::compile
from taking&mut self
to justself
feels unfortunate, since it forces callers to build a newTargetIsa
for every function they compile. What would you think of changingJITBuilder
to take a shared reference instead? Either&dyn TargetIsa
orRc<dyn TargetIsa>
, whichever is easier. In the latter case,SingleFunctionCompiler
would also take anRc
instead of aBox
.
jameysharp submitted PR review.
jameysharp created PR review comment:
What is this change for?
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.
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.
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?
afonso360 submitted PR review.
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.
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.
afonso360 submitted PR review.
jameysharp merged PR #4453.
Last updated: Dec 23 2024 at 13:07 UTC