Update reference types support to latest Binaryen #1465
Conversation
tests/compiler/std/set.optimized.wat
Outdated
| @@ -4251,14 +4250,57 @@ | |||
| i32.store offset=20 | |||
| local.get $0 | |||
| ) | |||
| (func $~lib/set/Set<u16>#find (param $0 i32) (param $1 i32) (param $2 i32) (result i32) | |||
There was a problem hiding this comment.
Wondering what happend here. @MaxGraey any ideas why this doesn't optimize away anymore?
There was a problem hiding this comment.
It just add one more monomorphized version for u16. I'm wondering could we rewrite Set#find in more sign agnostic way?
There was a problem hiding this comment.
Ah, I got why this happened. Before sign-extension $~lib/set/Set<u16>#find and $~lib/set/Set<i16>#find was pretty similar and folded (dedup) into one function, but with sign-extension they becomes looks pretty different and can't deduplicated anymore
There was a problem hiding this comment.
Ah, I see. Makes sense with the signed variant now utilizing sign extension while the unsigned variant does not. Too sad that it also doesn't optimize to the same code anymore.
tests/compiler/std/map.optimized.wat
Outdated
| i32.load8_u | ||
| i32.load8_s | ||
| local.get $3 | ||
| i32.const 255 | ||
| i32.and | ||
| i32.extend8_s |
There was a problem hiding this comment.
That's looks strange little bit. Is it correct changing load8_u to load8_s and zero-extension to sign-extension?
There was a problem hiding this comment.
Hmm, good question. Looks like it's picking the wrong sign extension instruction indeed.
There was a problem hiding this comment.
Every unsigned load and every zero-extensions was changed to signed versions. I don't think it's correct. Is it doing by binaryen, right?
There was a problem hiding this comment.
Might be a Binaryen optimization doing this, at least didn't find it in the untouched file on a first glimpse. Also, the only place we are emitting i32.extend8_s is in ensureSmallIntegerWrap on Type.i8, which seems correct.
There was a problem hiding this comment.
zero-extensions should leave as is. I don't understand why it forced always to sign-extensions
There was a problem hiding this comment.
However it doing only for cases like
(x & 0xFFFF) == y --> i32.extrend16_s(x) == y
i32.load16_u(...) == (y & 0xFFFF) --> i32.load16_s(...) == i32.extrend16_s(y)
which seems legit due to i32.eq / i32.ne is sign-agnostic
There was a problem hiding this comment.
Just to confirm: The optimization here is actually correct, so nothing to worry about?
There was a problem hiding this comment.
yes, it's correct in such context
| @@ -27,4 +250,7 @@ | |||
| i32.load8_s | |||
| drop | |||
| ) | |||
| (func $~start | |||
| call $start:retain-i32 | |||
| ) | |||
There was a problem hiding this comment.
retain-i32.optimized.wat also pretty huge regression
There was a problem hiding this comment.
Might this have something to do with sign extension ops not being recognized by precompute or something?
| local.tee $2 | ||
| i32.extend8_s | ||
| local.get $2 | ||
| i32.extend8_s | ||
| i32.ne |
There was a problem hiding this comment.
this definitely isolate expressions from pattern matching for x != x => false case. Btw great challenge for improve binaryen in this part.
|
Could you update binaryen dep again btw? |
|
Done, with a few fixture changes. |
|
Yes, it was expected. Now binary expressions with float constants also canonicalized always as |
|
@MaxGraey Are there remaining issues that need to be addressed in here, or is it ok to be merged for now? |
|
Could we wait for next nightly binaryen release tonight? It comes with couple optimization enhancements and I want to see how this affect for us |
|
Sure! :) |
|
Btw I'm not sure we should turn on sign-extention feature by default. Safari still doesn't support it |
|
Hmm, the matrix looks worrying in regards to when to enable a standardized feature, and from just looking at the table it seems that the difference in features supported by Safari will only get worse. |
|
Yes, that's need a discussion. |
|
Disabled sign extension for now since it's not crucial, but added an agenda item for discussion to the next meeting. |
|
Let me know if this looks good to you now :) |
|
🎉 This PR is included in version 0.14.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Let's see how this goes on CI :)