Skip to content

wasm2c: Cleanup of handling of WASM_RT_USE_SEGUE macro #2487

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

Jump to bottom
Merged
shravanrn merged 1 commit into from
Oct 15, 2024
Merged

wasm2c: Cleanup of handling of WASM_RT_USE_SEGUE macro #2487

shravanrn merged 1 commit into from
Oct 15, 2024

Conversation

@shravanrn
Copy link
Collaborator

@shravanrn shravanrn commented Oct 14, 2024 •
edited
Loading

The WASM_RT_USE_SEGUE macro was previously overloaded for two checks:

  1. Check if Segue is supported by the current compiler/arch/OS combination
  2. Check if the current module only uses one memory, as we do not use Segue for modules using multiple memories

Check 2 means that WASM_RT_USE_SEGUE is only available in the generated C file, and not in the runtime (wasm-rt-impl.c).

However, the runtime needs this information as it must include certain headers only if Segue is supported (Check 1 above), and thus this macro must be available in wasm-rt.h. We thus split this macro into two parts WASM_RT_USE_SEGUE and WASM_RT_USE_SEGUE_FOR_THIS_MODULE to address the two use cases: "check if Segue is supported" and "check if Segue should be used for the current module"

@shravanrn shravanrn requested review from keithw and sbc100 October 14, 2024 21:56
@sbc100
Copy link
Member

sbc100 commented Oct 14, 2024

Rather than creating a new WASM_RT_USE_SEGUE_FOR_THIS_MODULE for use in the generated code could we just undefine WASM_RT_USE_SEGUE when not IS_SINGLE_UNSHARED_MEMORY is not true?

@shravanrn
Copy link
Collaborator Author

shravanrn commented Oct 14, 2024

Rather than creating a new WASM_RT_USE_SEGUE_FOR_THIS_MODULE for use in the generated code could we just undefine WASM_RT_USE_SEGUE when not IS_SINGLE_UNSHARED_MEMORY is not true?

@sbc100 Sure, that would work too. I think, on balance, I think its a bit more confusing though.

Let me know if you have a strong preference for your approach, and I can make the change. If neutral, I prefer this as it is more explicit about the intent.

@sbc100
Copy link
Member

sbc100 commented Oct 14, 2024

Rather than creating a new WASM_RT_USE_SEGUE_FOR_THIS_MODULE for use in the generated code could we just undefine WASM_RT_USE_SEGUE when not IS_SINGLE_UNSHARED_MEMORY is not true?

@sbc100 Sure, that would work too. I think, on balance, I think its a bit more confusing though.

Let me know if you have a strong preference for your approach, and I can make the change. If neutral, I prefer this as it is more explicit about the intent.

Its up to you. This change LGTM as is if you prefer it that way. It makes the diff bigger but I guess that in an of itself is not a strong argument.

@shravanrn shravanrn merged commit 2b4e2c7 into WebAssembly:main Oct 15, 2024
@shravanrn shravanrn deleted the permodule-usesegue branch October 15, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbc100 sbc100 sbc100 approved these changes

@SoniEx2 SoniEx2 SoniEx2 approved these changes

@keithw keithw Awaiting requested review from keithw

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shravanrn @sbc100 @SoniEx2