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

deps: cherry-pick 5005faed5 from V8 upstream #15177

Closed

Conversation

kurayama
Copy link
Contributor

@kurayama kurayama commented Sep 4, 2017

Original commit message:

[turbofan] Improve representation selection for type guard.

This takes into account the type of the type guard when choosing
representation for a node. To make the representation changes
unambiguous, we pass the restricted type to the changer.

BUG=chromium:726554

Review-Url: https://codereview.chromium.org/2920193004
Cr-Commit-Position: refs/heads/master@{#45734}

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Sep 4, 2017
@kurayama
Copy link
Contributor Author

kurayama commented Sep 4, 2017

This cherry-pick fixes a bug that crashed our test suite with node>=8.3 when using the default v8 options (it only works with no-turbo)

@benjamingr
Copy link
Member

Thanks for making a contribution.

Is there any reason you didn't take the actual commit from V8? I think it's preferable to land the actual commit that landed in V8 so that when updating the version the commit will already be there.

Also pinging @nodejs/v8

@targos
Copy link
Member

targos commented Sep 4, 2017

@benjamingr What do you mean? This commit contains the exact same changes as the V8 commit. Anyway it doesn't matter for V8 updates because we just replace the deps/v8 directory with the new version.

@targos
Copy link
Member

targos commented Sep 4, 2017

@kurayama Thanks, this LGTM. Could you add the original commit message to your commit message too (with 4 spaces indent)? That would be perfect.

Original commit message:

    [turbofan] Improve representation selection for type guard.

    This takes into account the type of the type guard when choosing
    representation for a node. To make the representation changes
    unambiguous, we pass the restricted type to the changer.

    BUG=chromium:726554

    Review-Url: https://codereview.chromium.org/2920193004
    Cr-Commit-Position: refs/heads/master@{nodejs#45734}
@kurayama
Copy link
Contributor Author

kurayama commented Sep 4, 2017

@targos done

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Rubber-stampy LGTM

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

jasnell pushed a commit that referenced this pull request Sep 7, 2017
Original commit message:

    [turbofan] Improve representation selection for type guard.

    This takes into account the type of the type guard when choosing
    representation for a node. To make the representation changes
    unambiguous, we pass the restricted type to the changer.

    BUG=chromium:726554

    Review-Url: https://codereview.chromium.org/2920193004
    Cr-Commit-Position: refs/heads/master@{#45734}

PR-URL: #15177
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

Landed in 81b2a89

@jasnell jasnell closed this Sep 7, 2017
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Original commit message:

    [turbofan] Improve representation selection for type guard.

    This takes into account the type of the type guard when choosing
    representation for a node. To make the representation changes
    unambiguous, we pass the restricted type to the changer.

    BUG=chromium:726554

    Review-Url: https://codereview.chromium.org/2920193004
    Cr-Commit-Position: refs/heads/master@{#45734}

PR-URL: #15177
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
Original commit message:

    [turbofan] Improve representation selection for type guard.

    This takes into account the type of the type guard when choosing
    representation for a node. To make the representation changes
    unambiguous, we pass the restricted type to the changer.

    BUG=chromium:726554

    Review-Url: https://codereview.chromium.org/2920193004
    Cr-Commit-Position: refs/heads/master@{#45734}

PR-URL: #15177
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Original commit message:

    [turbofan] Improve representation selection for type guard.

    This takes into account the type of the type guard when choosing
    representation for a node. To make the representation changes
    unambiguous, we pass the restricted type to the changer.

    BUG=chromium:726554

    Review-Url: https://codereview.chromium.org/2920193004
    Cr-Commit-Position: refs/heads/master@{#45734}

PR-URL: #15177
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Original commit message:

    [turbofan] Improve representation selection for type guard.

    This takes into account the type of the type guard when choosing
    representation for a node. To make the representation changes
    unambiguous, we pass the restricted type to the changer.

    BUG=chromium:726554

    Review-Url: https://codereview.chromium.org/2920193004
    Cr-Commit-Position: refs/heads/master@{nodejs#45734}

PR-URL: nodejs#15177
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins
Copy link
Contributor

should this backport to v6.x?

@kurayama
Copy link
Contributor Author

@MylesBorins This only applies to turbofan which I believe is not present in node 6 (v8 5.1)

@kurayama kurayama deleted the fix-representation-selection branch February 18, 2019 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants