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

[v18.x] deps: V8: cherry-pick 9ec4e9095a25 #47092

Closed
wants to merge 1 commit into from
Closed

[v18.x] deps: V8: cherry-pick 9ec4e9095a25 #47092

wants to merge 1 commit into from

Conversation

kleisauke
Copy link

Original commit message:

[turbofan] Fix 32-to-64 bit spill slot moves

Other optimizations can create a situation where it is valid to treat a
stack slot as either 32-bit (which is what its value was created as) or
64-bit value (to which it was implicitly zero-extended). So when moving
such a value to a register, we cannot use a 32-bit move instruction just
because the source was annotated as such; we must also take the target
slot's representation into account.

Fixed: chromium:1407594
Bug: chromium:1356461
Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349
Auto-Submit: Jakob Kummerow <[email protected]>
Reviewed-by: Maya Lekova <[email protected]>
Commit-Queue: Maya Lekova <[email protected]>
Commit-Queue: Jakob Kummerow <[email protected]>
Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
Refs: #46446 (comment)

@kleisauke
deps: V8: cherry-pick 9ec4e9095a25
f571629 
Original commit message:

    [turbofan] Fix 32-to-64 bit spill slot moves

    Other optimizations can create a situation where it is valid to treat a
    stack slot as either 32-bit (which is what its value was created as) or
    64-bit value (to which it was implicitly zero-extended). So when moving
    such a value to a register, we cannot use a 32-bit move instruction just
    because the source was annotated as such; we must also take the target
    slot's representation into account.

    Fixed: chromium:1407594
    Bug: chromium:1356461
    Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349
    Auto-Submit: Jakob Kummerow <[email protected]>
    Reviewed-by: Maya Lekova <[email protected]>
    Commit-Queue: Maya Lekova <[email protected]>
    Commit-Queue: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@kleisauke kleisauke mentioned this pull request Mar 14, 2023
Closed
5 tasks
@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. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Mar 14, 2023
@atjn
Copy link

atjn commented Mar 22, 2023

@targos What do we need to do to get this merged?

I don't like to just tag people, but it looks like this patch has accidentally been overlooked. If there is a specific reason you are all ignoring it, please let us know :)
I am specifically asking you (@targos) since you seem to know your way around V8 patches and since you merged the patch that created this issue: #46446

Thanks!

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2023
@targos
Copy link
Member

targos commented Mar 22, 2023

@atjn It was probably just overlooked indeed. There's no rush, though, as a PR targeting an LTS release line, it would be looked at by a releaser when they prepare the next v18.x release

@atjn
Copy link

atjn commented Mar 22, 2023

I see, thanks again!

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@kleisauke
Copy link
Author

This issue also affects Node 19.x, I could open a similiar PR to the v19.x-staging branch if needed/appropriate.

danielleadams pushed a commit that referenced this pull request Apr 2, 2023
@kleisauke @danielleadams
deps: V8: cherry-pick 9ec4e9095a25
1ac639a 
Original commit message:

    [turbofan] Fix 32-to-64 bit spill slot moves

    Other optimizations can create a situation where it is valid to treat a
    stack slot as either 32-bit (which is what its value was created as) or
    64-bit value (to which it was implicitly zero-extended). So when moving
    such a value to a register, we cannot use a 32-bit move instruction just
    because the source was annotated as such; we must also take the target
    slot's representation into account.

    Fixed: chromium:1407594
    Bug: chromium:1356461
    Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349
    Auto-Submit: Jakob Kummerow <[email protected]>
    Reviewed-by: Maya Lekova <[email protected]>
    Commit-Queue: Maya Lekova <[email protected]>
    Commit-Queue: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
PR-URL: #47092
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danielleadams
Copy link
Contributor

Landed in 1ac639a

@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
@kleisauke kleisauke mentioned this pull request Apr 13, 2023
Closed
@kleisauke
Copy link
Author

This issue also affects Node 19.x, I could open a similiar PR to the v19.x-staging branch if needed/appropriate.

I just opened PR #47535 for this.

@kleisauke kleisauke deleted the cherrypick-v8-9ec4e9095a25-v18 branch April 13, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kleisauke @nodejs-github-bot @atjn @targos @danielleadams @richardlau