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.

Sign up for GitHub

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

Fallback to portable(-ish) SIMD reads against guard pages on not-yet-supported platforms #2545

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fallback to portable(-ish) SIMD reads against guard pages on not-yet-supported platforms #2545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f29995d
Switch to portable SIMD reads
SoniEx2 Feb 18, 2025
a4dc754
Alternative impl
SoniEx2 Feb 18, 2025
132d7a0
Only use volatile if we have no better option
SoniEx2 Mar 17, 2025
9e8e0f2
Use force-read only when necessary
SoniEx2 Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/prebuilt/wasm2c_simd_source_declarations.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
const char* s_simd_source_declarations = R"w2c_template(#if defined(__GNUC__) && defined(__x86_64__)
const char* s_simd_source_declarations = R"w2c_template(#if WASM_RT_MEMCHECK_GUARD_PAGES
)w2c_template"
R"w2c_template(#if defined(__GNUC__) && defined(__x86_64__)
)w2c_template"
R"w2c_template(#define SIMD_FORCE_READ(var) __asm__("" ::"x"(var));
)w2c_template"
Expand All @@ -8,6 +10,14 @@ R"w2c_template(#define SIMD_FORCE_READ(var) __asm__("" ::"w"(var));
)w2c_template"
R"w2c_template(#else
)w2c_template"
R"w2c_template(// best-effort using volatile
)w2c_template"
R"w2c_template(#define SIMD_FORCE_READ(var) (void)*(volatile v128*)&var;
)w2c_template"
R"w2c_template(#endif
)w2c_template"
R"w2c_template(#else
)w2c_template"
R"w2c_template(#define SIMD_FORCE_READ(var)
)w2c_template"
R"w2c_template(#endif
Expand Down
5 changes: 5 additions & 0 deletions src/template/wasm2c_simd.declarations.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
#if WASM_RT_MEMCHECK_GUARD_PAGES
#if defined(__GNUC__) && defined(__x86_64__)
#define SIMD_FORCE_READ(var) __asm__("" ::"x"(var));
#elif defined(__GNUC__) && defined(__aarch64__)
#define SIMD_FORCE_READ(var) __asm__("" ::"w"(var));
#else
// best-effort using volatile
#define SIMD_FORCE_READ(var) (void)*(volatile v128*)&var;
Copy link
Member

Choose a reason for hiding this comment

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

Why only with WASM_RT_MEMCHECK_GUARD_PAGES, why not make this the default fallback?

Copy link
Member

Choose a reason for hiding this comment

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

Can we use _asm__("" ::"x"(var)); on x86 too maybe?

Copy link
Collaborator

@shravanrn shravanrn Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we don't want it as the default fallback always because it may have some performance implications (we have to pay this in the guard model for spec compliance, but no reason to pay it when using bounds checks)

We should probably put the whole thing under the guard_pages macro check tbh

#if WASM_RT_MEMCHECK_GUARD_PAGES
#  if defined(__GNUC__) && defined(__x86_64__)
#    define SIMD_FORCE_READ(var) __asm__("" ::"x"(var));
#  elif defined(__GNUC__) && defined(__aarch64__)
#    define SIMD_FORCE_READ(var) __asm__("" ::"w"(var));
#  else
    // best-effort using volatile
#    define SIMD_FORCE_READ(var) (void)*(volatile v128*)&var;
#  endif
#else
#  define SIMD_FORCE_READ(var)
#endif

but this change would be just for clarity and would not result in perf difference

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would expect SIMD_FORCE_READ to not be used at all if its not needed.

Defining it to something that doesn't actually force anything seems wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SIMD_FORCE_READ_IF_NECESSARY?

Copy link
Member

Choose a reason for hiding this comment

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

Lets just leave the name as is.

I do this think we should not define it to anything unless WASM_RT_MEMCHECK_GUARD_PAGES is set though.

Copy link
Collaborator Author

@SoniEx2 SoniEx2 Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

that would make the code way messier tho... hmm...

SIMD_CHECK_PAGE_FAULT? (or maybe SIMD_CHECK_READ?) we already have bounds-check macros that do nothing when they're not needed...

#endif
#else
#define SIMD_FORCE_READ(var)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to have a macro called SIMD_FORCE_READ that doesn't force a read. We're already careful not to call this in the "checked" versions of the memory access functions.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed but:

(a) this a pre-existing condition that doesn't necessarily need to be fixed as part of this PR
(b) can you think of a better name that isn't overly long like SIMD_MAYBE_FORCE_READ or SIMD_FORCE_READ_IF_NECESSARY? Maybe we leave the bikeshedding for a separate PR/issue?

Copy link
Member

Choose a reason for hiding this comment

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

What's new in this PR (in the current version) is that if WASM_RT_MEMCHECK_GUARD_PAGES isn't set, now SIMD_FORCE_READ will become a nop -- that's the change that I didn't super-love.

I don't think we need a SIMD_MAYBE_FORCE_READ because we're already careful only to call this in the "unchecked" versions of all the memory ops.

Copy link
Member

Choose a reason for hiding this comment

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

I see, so maybe WASM_RT_MEMCHECK_GUARD_PAGES isn't set we simply don't need to define SIMD_FORCE_READ at all?

Copy link
Collaborator Author

@SoniEx2 SoniEx2 Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

keep in mind the "unchecked" versions are the base versions. they're used regardless of page size, so even tho custom-page-sizes does manual bounds checking, it's still additionally triggering this force-read when guard pages are enabled. (even tho custom-page-sizes doesn't/can't use guard pages, yes.)

how about we rename it to SIMD_CHECK_READ? it'd be nice to only apply this to the default32 version, but that's... difficult, currently. (also at some point we are getting too deep into C preprocessor metaprogramming, would it be a good idea to expand the template system a little bit and generate slightly more pre-expanded versions of these macros?)

#endif
// TODO: equivalent constraint for ARM and other architectures
Expand Down
Loading