Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Note newly enabled features in LLVM in ChangeLog #21853

Merged
merged 2 commits into from Apr 30, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Apr 29, 2024

llvm/llvm-project#80923 enabled multivalue and reference-types by default. Even though this is an LLVM change, given that we release emsdk with all LLVM/Binaryen/Emscripten versions, it could be worth mentioning in the ChangeLog.

llvm/llvm-project#80923 enabled multivalue and
reference-types by default. Even though this is an LLVM change, given
that we release emsdk with all LLVM/Binaryen/Emscripten versions, it
could be worth mentioning in the ChangeLog.
@aheejin aheejin requested review from kripken and sbc100 April 29, 2024 21:30
ChangeLog.md Outdated
@@ -24,6 +24,9 @@ See docs/process.md for more on how version tagging works.
of pthread builds so that is generated alongside the main JavaScript file.
See #21701. ()
- `-sASYNCIFY=2` is setting now deprecated, use `-sJSPI` instead.
- LLVM now enables multivalue and reference-types features by default. Enabling
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps phrase this as something that emscripten now supports? Maybe "Due to and upstream llvm change, the x and y features are not enabled by default in emscripten"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried doing that.

@kleisauke
Copy link
Collaborator

Should we also update MIN_NODE_VERSION to 170200 since reference-types requires that version?

var MIN_NODE_VERSION = 160000;

@sbc100
Copy link
Collaborator

sbc100 commented Apr 30, 2024

Should we also update MIN_NODE_VERSION to 170200 since reference-types requires that version?

var MIN_NODE_VERSION = 160000;

Good point. Maybe we should just go to 18 since that is LTS.

Do all our min browser versions support reference types?

@kleisauke
Copy link
Collaborator

Do all our min browser versions support reference types?

Ah, it looks like MIN_CHROME_VERSION and MIN_SAFARI_VERSION should also be adjusted to 96 and 150000, respectively.

var MIN_CHROME_VERSION = 85;

var MIN_SAFARI_VERSION = 140100;

See: https://caniuse.com/wasm-reference-types

@sbc100
Copy link
Collaborator

sbc100 commented Apr 30, 2024

Do all our min browser versions support reference types?

Ah, it looks like MIN_CHROME_VERSION and MIN_SAFARI_VERSION should also be adjusted to 96 and 150000, respectively.

var MIN_CHROME_VERSION = 85;

var MIN_SAFARI_VERSION = 140100;

See: https://caniuse.com/wasm-reference-types

Yeah, I'm not totally sure we need/want to do this. Especially since we are not going to actually be generating any reference types by default.

@aheejin
Copy link
Member Author

aheejin commented Apr 30, 2024

I'll then land this for now.

@aheejin aheejin merged commit 5654715 into emscripten-core:main Apr 30, 2024
29 checks passed
@aheejin aheejin deleted the enable_features branch April 30, 2024 18:28
aheejin added a commit to aheejin/emscripten that referenced this pull request May 8, 2024
I submitted emscripten-core#21853 before landing
llvm/llvm-project#80923, so the previous version
(3.1.59) was released before the LLVM change actually landed. And then
we decided to disable reference-types temporarily
(llvm/llvm-project#90792) while the node version
is being resolved
(emscripten-core/emsdk#1173 (comment)).

This moves the entry to the current release and only mentions multivalue
is the newly enabled feature.
aheejin added a commit that referenced this pull request May 8, 2024
I submitted #21853 before landing
llvm/llvm-project#80923, so the previous version
(3.1.59) was released before the LLVM change actually landed. And then
we decided to disable reference-types temporarily
(llvm/llvm-project#90792) while the node version
is being resolved
(emscripten-core/emsdk#1173 (comment)).

This moves the entry to the current release and only mentions multivalue
is the newly enabled feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants