search

WebAssembly / wabt

The WebAssembly Binary Toolkit
Apache License 2.0
6.91k stars 702 forks source link

wasm2c: Cleanup of handling of WASM_RT_USE_SEGUE macro #2487

Closed shravanrn closed 1 month ago

shravanrn commented 1 month ago

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"

sbc100 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.