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.

Sign up for GitHub

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

Jump to bottom

v8,tools: expose experimental wasm revectorize feature #54896

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

v8,tools: expose experimental wasm revectorize feature #54896

wants to merge 2 commits into from
+20 −1

Conversation

yolanda15
Copy link

@yolanda15 yolanda15 commented Sep 12, 2024 •
edited
Loading

The wasm simd256 revectorization feature is enabled by default when v8_enable_webassembly is true on x64 target in V8. At runtime it will be enabled through flag --experimental-wasm-revectorize.

This PR adds the build configuration in Node to enable this feature and make it explicitly accessible through the same runtime flag for x64 build.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Sep 12, 2024
targos
targos reviewed Sep 12, 2024
View reviewed changes
configure.py Outdated Show resolved Hide resolved
@yolanda15
Copy link
Author

yolanda15 commented Sep 20, 2024 •
edited
Loading

@targos Could you help me merge this PL? I seem not authorized and it's already one week passed. Thank you in advance!

@gengjiawen gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 20, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 20, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54896
✔  Done loading data for nodejs/node/pull/54896
----------------------------------- PR info ------------------------------------
Title      v8,tools: expose experimental wasm revectorize feature (#54896)
Author     Yolanda-Chen <[email protected]> (@yolanda15, first-time contributor)
Branch     yolanda15:enable_revec -> nodejs:main
Labels     build, v8 engine, tools, needs-ci
Commits    1
 - v8,tools: expose experimental wasm revectorize feature
Committers 1
 - Yolanda Chen <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54896
Reviewed-By: Michaël Zasso <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54896
Reviewed-By: Michaël Zasso <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 12 Sep 2024 07:48:42 GMT
   ✔  Approvals: 1
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/54896#pullrequestreview-2303482481
   ✔  Last GitHub CI successful
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10952384360

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62635/

@yolanda15
Copy link
Author

The test failure seems unrelated. Not wasm test and not reproducible locally. Rebased and rerun the test.

@yolanda15
Copy link
Author

Rebase again. Could anyone help approve the workflows?

@RedYetiDev RedYetiDev removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 26, 2024
@yolanda15
Copy link
Author

Seems all test passed. I'm still not authorized. Could anyone help merge this PR? Not sure if any other authority is needed. Thanks in advance!

@marco-ippolito marco-ippolito added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 2, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63389/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6265/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6265/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6265/

@yolanda15
Copy link
Author

Thanks Marco! Rebased to avoid test failures.

@aduh95
Copy link
Contributor

aduh95 commented Nov 5, 2024

Thanks Marco! Rebased to avoid test failures.

I would recommend to not rebase, unless there are conflicts to fix: when you rebase, the PR requires a full CI re-run, while we can simply resume the failed test suite if you don't rebase.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63436/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6270/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6270/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6270/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63444/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63446/

@yolanda15
Copy link
Author

yolanda15 commented Nov 6, 2024 •
edited
Loading

I would recommend to not rebase, unless there are conflicts to fix: when you rebase, the PR requires a full CI re-run, while we can simply resume the failed test suite if you don't rebase.

Thanks for the recommendation! Noted and will avoid the rebase next time. @aduh95

@yolanda15
Copy link
Author

Added a new commit to disable revec on mac to avoid a bulid failure with clang13 on macOS.
Please see failed log at https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx11-x64/61922/consoleFull. The same file compiles on Linux with g++-12.
@targos @Jasnel Please help take a look. Thanks!

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63453/

@yolanda15
Copy link
Author

CI: https://ci.nodejs.org/job/node-test-pull-request/63453/

The test failure on node-test-commit-arm seems timeout:

21:16:03 not ok 4128 parallel/test-stream-readable-unpipe-resume
21:16:03 ---
21:16:03 duration_ms: 360064.83100
21:16:03 severity: fail
21:16:03 exitcode: -15
21:16:03 stack: |-
21:16:03 timeout
21:16:03 ...

May need a retest @aduh95. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yolanda15 @nodejs-github-bot @aduh95 @jasnell @targos @gengjiawen @marco-ippolito @RedYetiDev