Fix issue around i64 neg one check in binaryen.js / asmjs

[MaxGraey]

Since #2870 I refactored this line: https://github.com/WebAssembly/binaryen/pull/2870/files#diff-54e02686df3480c98e3f90a24705c00bR1395

from

if (right->value == Literal(int32_t(-1)) ||
    right->value == Literal(int64_t(-1))) {

to

auto constRight = right->value.getInteger();
if (constRight == -1LL) {

and it's working for native version as you can see in extra tests here for example:

(drop (i64.le_u    ;; skip
  (local.get $y)
  (i64.const 4294967295)
))

don't optimize to i64.const 1 but if you build same example via binaryen.js this happening. Accidentally 4294967295 constant in binaryen.js interprets as u64(-1) and test above folded to:

(drop (i32.const 1))

Which is wrong and valid only for (i32.const 4294967295) but not for (i64.const 4294967295).

I'm not sure how better fix this. We could revert old approach with checking Literal values. Or may be it's edge case of i64-to-i32-lowering or something other which relate to wasm2js and better fix that pass?

cc @kripken

[kripken]

It's good we found this - let's fix the underlying issue though, instead of working around it here. (Do we have current breakage that we need to fix urgently, though? If so we may want to land this, or revert the PR, or something else for now.)

Can you make a standalone testcase in C or Cpp that shows the problem? Then I can debug that.

[MaxGraey]

Can you make a standalone testcase in C or Cpp that shows the problem? Then I can debug that.

Hmm, I will try. My guess it somehow related to process how union with int64 lowering in asm.js

union Integer {
   int32_t i32;
   int64_t i64;
}

Integer val = {-1};
val.i64 = -1LL;

assert(val.i64 != 4294967295LL); // is it pass?
assert(val.i64 == -1LL);

But I'm not sure

[MaxGraey]

No, unfortunately I still can't isolate this issue to minimal example.

[MaxGraey]

But I can confirm this PR fix the problem, so will be great merge it as fast workaround

[kripken]

What's a minimal .wat file to show the issue, at least - that would help too? From the description it sounds like

(drop (i64.le_u    ;; skip
  (local.get $y)
  (i64.const 4294967295)
))

Or something like that, but would be good to have a full .wat file here, with the expected and incorrect output.

[MaxGraey]

Or something like that, but would be good to have a full .wat file here, with the expected and incorrect output.

yes, It's minimal example for binaryen.js. So native binaryen don't tuch this and optimize to

(drop (i64.le_u
  (local.get $y)
  (i64.const 4294967295)
))

but binaryen.js unexpectedly fold it to

(drop (i32.const 1))

Which should valid only for (i64.const -1) or (i32.const -1) (same as 4294967295)

[kripken]

I see, thanks @MaxGraey

Ok, with that I could reproduce the problem, and then I spent some time reducing it, ending up with this bug report in LLVM: https://bugs.llvm.org/show_bug.cgi?id=46447 - this looks like an LLVM wasm backend codegen issue. Very good we found it!

[MaxGraey]

@kripken @tlively As I understand this will not be quick fix. First of all it should fixed on LLVM, than Binaryen should switch to this LLVM version. Could we merge this workaround as is but with // TODO: revert changes after fix https://bugs.llvm.org/show_bug.cgi?id=46447 comment?

[kripken]

If there's not a quick fix on the LLVM side then I agree.

But if there is, then waiting for that isn't so bad as emscripten updates to latest LLVM very quickly - we can make sure to tag a release right after.

So it depends on what @tlively says about the LLVM status I think.

[MaxGraey]

Ah, good to know binaryen can fast migrate to newer LLVM

[tlively]

I have a fix up for review at https://reviews.llvm.org/D83017, so this shouldn’t be a problem much longer.

[MaxGraey]

@tlively Thanks!

[MaxGraey]

Fix already in upstream of LLVM. Good job @tlively !

[kripken]

Does the binaryen.js buildbot use the tot build? Then it should already have the fix.

If it uses a release, then I can tag a release after the weekend.

[dcodeIO]

Yep, it uses tot by default.

[kripken]

Ok great, then this PR should not be needed anymore (please confirm).

Thanks again for filing this @MaxGraey - if this LLVM bug wasn't found on binaryen, it might have affected other projects that are much harder (closed source, bigger, etc.) for us to get a useful testcase from!

[MaxGraey]

Yes, everything worked out very well. Thank you all for your quick and efficient fix! Will close this PR tomorrow after check new binaryen.js release.

[MaxGraey]

@kripken

If it uses a release, then I can tag a release after the weekend.

It seems binaryen.js bot required new commit or release of emscripten. Without this it download this most recent version:
emcc (Emscripten gcc/clang-like replacement) 1.39.18 (2bee818f21ff1687f28a4186795c10509cea784c) and don't see any reason for publish new nightly release unfortunately.

[dcodeIO]

Binaries the bot used upon emsdk install tot last run:

https://storage.googleapis.com/webassembly/emscripten-releases-builds/linux/99913ebee94cf357e1df3bbe7e021f83e91ebc5c/wasm-binaries.tbz2

[tlively]

Our tree is broken for other reasons, so the fix will not roll into tot until tomorrow probably

[MaxGraey]

Can confirm LLVM fix works and recent binaryen.js already shipped with proper switch logic. @tlively thanks again! Close this