[WIP] [opt] let OptimizeAddedConstants handle memory64 well

[dongAxis]

hi all,

When program is compile with wasm64, the addr and offset of load and store should be i64.

But OptimizeAddedConstants dose not handle well, so I use this patch to fix that.
Sample wasm will attached.

Best Regards
Yifeng

[dongAxis]

Since it is wip MR, so i do not write unit test. If this patch accept, I will write more unit test

[dongAxis]

attached sample file.
When we use binaryen to optimize this file, we will got an assert failed.
cmd line: wasm-opt -- --strip-dwarf --post-emscripten -O3 --low-memory-unused --zero-filled-memory --strip-producers a.out.wasm -o a.out.wasm.opt --detect-features
The backtrace will look like:

[kripken]

Thanks!

In addition to the comments below please also add a test. Perhaps in a new file, test/lit/passes/optimize-added-constants-memory64.wast.

[MATRIXKOO]

LGTM

[MATRIXKOO]

LGTM

[MATRIXKOO]

// /src/src/passes/OptimizeAddedConstants.cpp:242:15 looks like `value >=0` always true.
 uint64_t value = literal.getInteger();
 // Avoid uninteresting corner cases with peculiar offsets.
 if (value >= 0 && value < PassOptions::LowMemoryBound) {

[dongAxis]

@kripken In addition, could you give me a write permission? Because we will focus on binaryen and wasm, I would love to write more code for binaryen. Thanks in advance.

[dongAxis]

@kripken In addition, could you give me a write permission? Because we will focus on binaryen and wasm, I would love to write more code for binaryen. Thanks in advance.

Hi kripken, I found some of unit test failed, could you tell me how to run those tests in my local environment? thanks

[kripken]

Hmm, the CI error looks unrelated to this PR. It's a closure error,

building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!

Likely that is due to @sbc100's recent changes on emscripten (CI here uses latest unstable emscripten). Does that sound likely @sbc100 ?

@dongAxis In general though to run those tests you can look at .github/workflows/ci.yml, and I think it just installs the emsdk and runs ./scripts/emcc-tests.sh. But in this case we can ignore that failure.

About write access, I think the main issue was that github wouldn't run CI for you without someone approving it? That seems to have been fixed after your earlier commits that landed - I didn't need to manually run CI here today. I believe github requires manual approval for CI only for people that haven't committed to a repo.

[sbc100]

Hmm, the CI error looks unrelated to this PR. It's a closure error,

building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!

Likely that is due to @sbc100's recent changes on emscripten (CI here uses latest unstable emscripten). Does that sound likely @sbc100 ?

Yup that sounds expected. I guess we should either fix the warnings of build with -Wno-closure to disable them.

[dongAxis]

Hmm, the CI error looks unrelated to this PR. It's a closure error,

building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!

Likely that is due to @sbc100's recent changes on emscripten (CI here uses latest unstable emscripten). Does that sound likely @sbc100 ?

@dongAxis In general though to run those tests you can look at .github/workflows/ci.yml, and I think it just installs the emsdk and runs ./scripts/emcc-tests.sh. But in this case we can ignore that failure.

About write access, I think the main issue was that github wouldn't run CI for you without someone approving it? That seems to have been fixed after your earlier commits that landed - I didn't need to manually run CI here today. I believe github requires manual approval for CI only for people that haven't committed to a repo.

ok thanks

[kripken]

About memory64 performance, this PR fixes one important issue, and there are likely more places we just support 32-bit integers but not 64. I bet in OptimizeInstructions for example we have such issues. One way to find them might be to compile the same code in wasm32 and wasm64 and just look at the code side by side - anywhere the wasm64 code is larger is worth investigating.

cc @sbc100