Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore compatibility with upstream disas tests #135

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

frank-emrich
Copy link

@frank-emrich frank-emrich commented Mar 19, 2024

It recently became necessary to regenerate the expected output of most (all?) of the files in tests/disas. Concretely, we were generating slightly different CLIF than upstream even on modules not involving any WasmFX instructions.

This PR rectifies this situation, making sure that we generate identical CLIF again. This required two changes:

Global CLIF variable for runtime limits pointer

Since #99, we called the function wasmtime_cranelift::func_environ::FuncEnvironment::declare_vmruntime_limits_ptr unconditionally for every compiled function, meaning that every function prelude now contained a definition of the corresponding global variable.

This PR changes this behavior: I restored the original logic for when to call declare_vmruntime_limits_ptr. Instead of using the global variable vmruntime_limits to access the VMRuntimeLimits, we now load this pointer ourselves once per resume instruction. Since the load instruction is marked as readonly, it should be coalesced into a single load, even if multiple resumes appear in the same function.

I've benchmarked this, using a micro-benchmark with 2 resume instructions in the same function, and as expected there is no observable difference in performance.

Order of libcall declarations

In builtin.rs, we put the declarations of our own libcalls into the middle of the list of all libcalls in the foreach_builtin_function macro. This shifted the indices of some existing libcalls, which caused the CLIF to differ when using these. I've moved all the tc_ libcalls to the end of the list, thus leaving the indices of existing libcalls unchanged.

As a result, I've replaced all files in tests/disas with the versions from upstream (bytecodealliance/wasmtime@afaf1c7, the most recent upstream commit that we've merged)

@frank-emrich frank-emrich requested a review from dhil March 19, 2024 18:09
Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@frank-emrich frank-emrich merged commit cff2e76 into wasmfx:main Mar 19, 2024
19 checks passed
@frank-emrich frank-emrich deleted the no-runtime-limits-global branch March 19, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants