Bulk memory is enabled in -O3 even when asked not to.

[juj]

If one runs

emcc test\hello_world.c -o a.js -O3 -mno-bulk-memory -mno-bulk-memory-opt

the resulting code has a bulk memory Data Count section. See #25466

[sbc100]

It seems like we are lowering the actual instructions away --llvm-memory-copy-fill-lowering but binaryen is still including the data count section.

[kripken]

Binaryen will only emit it when the feature is enabled, so perhaps we are setting it for some reason?

https://github.com/WebAssembly/binaryen/blob/7ba340a934d17c3d8fc23ef9cd9f99e4e3ac4367/src/wasm/wasm-binary.cpp#L647-L654

[kripken]

(Also, --llvm-memory-copy-fill-lowering disables it, https://github.com/WebAssembly/binaryen/blob/7ba340a934d17c3d8fc23ef9cd9f99e4e3ac4367/src/passes/LLVMMemoryCopyFillLowering.cpp#L75 and https://github.com/WebAssembly/binaryen/blob/7ba340a934d17c3d8fc23ef9cd9f99e4e3ac4367/src/passes/LLVMMemoryCopyFillLowering.cpp#L114 )

[juj]

Binaryen will only emit it when the feature is enabled, so perhaps we are setting it for some reason?

https://github.com/WebAssembly/binaryen/blob/7ba340a934d17c3d8fc23ef9cd9f99e4e3ac4367/src/wasm/wasm-binary.cpp#L647-L654

From what I am seeing, Binaryen is scanning the .wasm file for what feature it has, and propagate the same features into the output. It is seeing the input .wasm file to have bulk-memory enabled, so it passes --enable-bulk-memory into the output:

C:\emsdk\emscripten\main>emcc test\hello_world.c -o a.js -O3 -mno-bulk-memory -mno-bulk-memory-opt

shared:DEBUG: successfully executed 'C:/emsdk/llvm/git/build_main_vs2022_64/Release/bin\wasm-ld.exe' -o a.wasm 'C:\Users\clb\AppData\Local\Temp\emscripten_temp\hello_world.o' '-LC:\emsdk\emscripten\main\cache\sysroot\lib\wasm32-emscripten' '-LC:\emsdk\emscripten\main\src\lib' -lGL-getprocaddr -lal -lhtml5 -lstubs -lnoexit -lc -ldlmalloc -lcompiler_rt -lc++-noexcept -lc++abi-noexcept -lsockets -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr 'C:\Users\clb\AppData\Local\Temp\emscripten_temp\tmp7k4yg0vulibemscripten_js_symbols.so' --strip-debug --export=_emscripten_stack_alloc --export=__wasm_call_ctors --export=emscripten_stack_get_current --export=_emscripten_stack_restore --export-if-defined=__start_em_asm --export-if-defined=__stop_em_asm --export-if-defined=__start_em_lib_deps --export-if-defined=__stop_em_lib_deps --export-if-defined=__start_em_js --export-if-defined=__stop_em_js --export-if-defined=main --export-if-defined=__main_argc_argv --export-table -z stack-size=65536 --no-growable-memory --initial-heap=16777216 --no-entry --table-base=1 --global-base=1024

emscripten:DEBUG: Metadata: {'all_exports': ['memory',
                 '__wasm_call_ctors',
                 'main',
                 '__indirect_function_table',
                 '_emscripten_stack_restore',
                 '_emscripten_stack_alloc',
                 'emscripten_stack_get_current'],
 'em_asm_consts': {},
 'em_js_func_types': {},
 'em_js_funcs': {},
 'features': ['--enable-bulk-memory',
              '--enable-bulk-memory-opt',
              '--enable-call-indirect-overlong',
              '--enable-multivalue',
              '--enable-mutable-globals',
              '--enable-nontrapping-float-to-int',
              '--enable-reference-types',
              '--enable-sign-ext'],

profiler:DEBUG: block "get_metadata" took 0.003 seconds

link:DEBUG: using binaryen
link:DEBUG: lowering bulk-memory feature due to incompatible target browser engines
shared:DEBUG: successfully executed 'C:/emsdk/binaryen/main_vs2022_64bit_binaryen\bin\wasm-opt.exe' --version
shared:DEBUG: successfully executed 'C:/emsdk/binaryen/main_vs2022_64bit_binaryen\bin\wasm-opt' --strip-target-features --llvm-memory-copy-fill-lowering --post-emscripten -O3 --low-memory-unused --zero-filled-memory --pass-arg=directize-initial-contents-immutable --no-stack-ir a.wasm -o a.wasm --mvp-features --enable-bulk-memory --enable-bulk-memory-opt 

Finally, it does lower, but the final output still has a Data Count section, which older browsers abort on.

[kripken]

If there is a features section, it is read and used, but the lowering pass should still disable it, so that's odd.

But as a quick test, removing the features section early might be interesting to try. (Though maybe we already do that?)

[juj]

I did something along those lines in this PR: #25466 , but there was doubt if that was the way to go. (It did pass the added test there though)

[sbc100]

I think the problem is that when we strip the feature section we remember the features in BINARYEN_FEATURES and then they get passed to future binaryen calls.

Do I think the trick is to prune BINARYEN_FEATURES at the same time that we run the lowering passes. i.e. I think @juj's change is right, but maybe just in the wrong place.

[juj]

I wonder if we might be able to land #25466 as a short-term solution? That would help me unblock testing through any failures on older browsers.