Skip to content

Conversation

@palmer-dabbelt
Copy link

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_t for 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

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
Copy link

ghost commented Jul 21, 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 ghost merged commit 9c45612 into riscv-collab:riscv-porting-dev 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants