forked from v8/v8
-
Notifications
You must be signed in to change notification settings - Fork 31
Fix the MIPS-style multiplication routines #37
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mulh on RISC-V doesn't work like mulh on MIPS: the rv64 mulh produces the top 64 bits of a 128-bit multiplication, as opposed to the top 32 bits of a 64-bit multiplication. The simulator followed the old behavior, which is incorrect on RISC-V. This patch fixes both the simulator's implementation of mulh and the compiler's one use of mulh to match the ISA's definition. I haven't run it on the hardware, but it should improve things. Additionally, I fixed the simulator's mul implementation to perform a 64-bit multiplication, which matches the ISA manual. I've kept these two changes in the same patch because the mul changes is necessary for the new Mulh to function, which is necessary to handle the mulh change. It doesn't clean up the larger issues in the port related to mulh, as now we're performing all 64-bit multiplications twice and throwing away half of the result each time. I'm trying to split up the bug fixes from the cleanups. Signed-off-by: Palmer Dabbelt <[email protected]>
This is essentially the same issue as the mulh issue. Since there is no 32x32->64-bit unsigned multiply on rv64 we instead use mulhu. This is slightly more efficient because we only need one shift per input instead of two (to zero extend). Signed-off-by: Palmer Dabbelt <[email protected]>
At this point this one is probably correct, but I figured it's just easier to clean it up as well. Signed-off-by: Palmer Dabbelt <[email protected]>
ghost
suggested changes
Jul 20, 2020
|
Since the CI was not correctly when it was triggered here, I am running the tests locally and will merge once confirmed that they all pass. |
ghost
approved these changes
Jul 22, 2020
ghost
pushed a commit
that referenced
this pull request
Aug 6, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Aug 28, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Oct 14, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Nov 11, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
luyahan
pushed a commit
to luyahan/v8
that referenced
this pull request
Nov 30, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes riscv-collab#37
ghost
pushed a commit
that referenced
this pull request
Dec 3, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Dec 9, 2020
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Jan 13, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Jan 26, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
luyahan
pushed a commit
to luyahan/v8
that referenced
this pull request
Jan 27, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes riscv-collab#37
luyahan
pushed a commit
to luyahan/v8
that referenced
this pull request
Feb 1, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes riscv-collab#37
ghost
pushed a commit
that referenced
this pull request
Feb 1, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
ghost
pushed a commit
that referenced
this pull request
Feb 3, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
luyahan
pushed a commit
to luyahan/v8
that referenced
this pull request
Feb 7, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes riscv-collab#37
luyahan
pushed a commit
to luyahan/v8
that referenced
this pull request
Feb 9, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes riscv-collab#37
ghost
pushed a commit
that referenced
this pull request
Feb 9, 2021
This fixes the errors with the `machops` tests related to `Ror`. Fixes #37
luyahan
pushed a commit
to luyahan/v8
that referenced
this pull request
Sep 2, 2025
…eath" This reverts commit e3f33e9. Reason for revert: https://crbug.com/398999390 This only reverts the fist part of the CL (Removing IsOneByteRepresentation). Using static roots to determine one-byte representation from the map word alone is kept. Original change's description: > [string] Remove String::IsOneByteRepresentationUnderneath > > All string maps are split into one- and two-byte versions, and enforce > that their underlying string must have the same byte width (e.g. we > can't change byteness when externalizing). This means that the old > IsOneByteRepresentationUnderneath loop is no longer needed, as we > already know the byteness from the top. > > Remove this function, replacing with calls to IsOneByteRepresentation. > Additionally, we have some static roots hacks to determine one-byte > representation from the map word alone (without needing to dereference > the instance type), so use that too where possible. > > Change-Id: I662daa3cb217d04cbfd717da53ae1238a0bcddd0 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6011010 > Commit-Queue: Leszek Swirski <[email protected]> > Reviewed-by: Patrick Thier <[email protected]> > Cr-Commit-Position: refs/heads/main@{#97099} DISABLE_SPELLCHECKER Fixed: 398999390 (cherry picked from commit 8db16e6) Change-Id: I2c84f730aa0527b24a3c74090aa92d689cdbe701 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6330265 Commit-Queue: Leszek Swirski <[email protected]> Auto-Submit: Patrick Thier <[email protected]> Reviewed-by: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/branch-heads/13.4@{riscv-collab#37} Cr-Branched-From: 0f87a54-refs/heads/13.4.114@{riscv-collab#1} Cr-Branched-From: 27af2e9-refs/heads/main@{#98459}
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looks like when the simulator was converted over from the MIPS port the multiplication routines ended up incorrectly implemented. This patch set fixes the routines and the associated issues that show up when running the test suite. I've used
__int128_tfor the multiplication routines, which probably isn't the best idea but there's already one instance of it in v8 so I figure it's OK for now.The RISC-V backend retains the MIPSisms, converting them to RISC-V instructions in the macro assembler. This results in some pretty inefficient code generation, but I want to go through the whole simulator first as I saw some more potential issues floating around. I figure it's more important to get this running on hardware than to clean up the code generation.
This supersedes #28