Skip to content

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)

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

@nodejs-github-botnodejs-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!

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

@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-actionsgithub-actionsbot 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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@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
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

@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.

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

Labels

buildIssues and PRs related to build files or the CI.needs-ciPRs that need a full CI run.v8 engineIssues and PRs related to the V8 dependency.v18.xIssues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@kleisauke@nodejs-github-bot@atjn@targos@danielleadams@richardlau