Skip to content

cmd/compile: the loong64 intrinsic for CompareAndSwapUint32 function needs to sign extend its "old" argument. [1.18 backport] #57344

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
Closed
gopherbot opened this issue Dec 15, 2022 · 6 comments
Closed

cmd/compile: the loong64 intrinsic for CompareAndSwapUint32 function needs to sign extend its "old" argument. [1.18 backport] #57344

gopherbot opened this issue Dec 15, 2022 · 6 comments
Labels
arch-mips arch-riscv Issues solely affecting the riscv64 architecture. CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Milestone
Go1.18.10

Comments

@gopherbot
Copy link
Contributor

@dr2chase requested issue #57282 to be considered for backport to the next 1.18 minor release.

@gopherbot please open backport issues.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Dec 15, 2022
@gopherbot gopherbot mentioned this issue Dec 15, 2022
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 15, 2022
@gopherbot gopherbot added this to the Go1.18.10 milestone Dec 15, 2022
@dr2chase
Copy link
Contributor

Loong64 isn't supported by 1.18, so not a bug.

@dr2chase
Copy link
Contributor

The three bugs/CLs were collapsed into one, and this is the one. Therefore it is the backport, though loong64 code won't be included in the 1.18 backport CL.

@dr2chase dr2chase reopened this Dec 17, 2022
@xen0n
Copy link
Member

xen0n commented Dec 17, 2022

Hi, what should we (loong64 porters) do at this point regarding this backport issue? Do we have to submit the backport CL now (or relatively quickly), or are you just planning to triage and decide on this later?

@dr2chase
Copy link
Contributor

Hi, I'll get it this time (and sorry for the late reply; I was traveling, then the weekend.)
Thank you for checking, though.

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/458357 mentions this issue: [release-branch.go1.18] cmd/compile: sign-extend the 2nd argument of the LoweredAtomicCas32 on mips64x,riscv64

@dr2chase dr2chase added the CherryPickApproved Used during the release process for point releases label Dec 21, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Dec 21, 2022
gopherbot pushed a commit that referenced this issue Dec 28, 2022
@dr2chase @gopherbot
[release-branch.go1.18] cmd/compile: sign-extend the 2nd argument of …
eeaf508 
…the LoweredAtomicCas32 on mips64x,riscv64

The function LoweredAtomicCas32 is implemented using the LL-SC instruction pair
on loong64, mips64x, riscv64. However,the LL instruction on loong64, mips64x,
riscv64 is sign-extended, so it is necessary to sign-extend the 2nd parameter
"old" of the LoweredAtomicCas32, so that the instruction BNE after LL can get
the desired result.

The function prototype of LoweredAtomicCas32 in golang:
    func Cas32(ptr *uint32, old, new uint32) bool

When using an intrinsify implementation:
    case 1: (*ptr) <= 0x80000000 && old < 0x80000000
        E.g: (*ptr) = 0x7FFFFFFF, old = Rarg1= 0x7FFFFFFF

        After run the instruction "LL (Rarg0), Rtmp": Rtmp = 0x7FFFFFFF
        Rtmp ! = Rarg1(old) is false, the result we expect

    case 2: (*ptr) >= 0x80000000 && old >= 0x80000000
        E.g: (*ptr) = 0x80000000, old = Rarg1= 0x80000000

        After run the instruction "LL (Rarg0), Rtmp": Rtmp = 0xFFFFFFFF_80000000
        Rtmp ! = Rarg1(old) is true, which we do not expect

When using an non-intrinsify implementation:
    Because Rarg1 is loaded from the stack using sign-extended instructions
    ld.w, the situation described in Case 2 above does not occur

Benchmarks on linux/loong64:
name     old time/op  new time/op  delta
Cas      50.0ns ± 0%  50.1ns ± 0%   ~     (p=1.000 n=1+1)
Cas64    50.0ns ± 0%  50.1ns ± 0%   ~     (p=1.000 n=1+1)
Cas-4    56.0ns ± 0%  56.0ns ± 0%   ~     (p=1.000 n=1+1)
Cas64-4  56.0ns ± 0%  56.0ns ± 0%   ~     (p=1.000 n=1+1)

Benchmarks on Loongson 3A4000 (GOARCH=mips64le, 1.8GHz)
name     old time/op  new time/op  delta
Cas      70.4ns ± 0%  70.3ns ± 0%   ~     (p=1.000 n=1+1)
Cas64    70.7ns ± 0%  70.6ns ± 0%   ~     (p=1.000 n=1+1)
Cas-4    81.1ns ± 0%  80.8ns ± 0%   ~     (p=1.000 n=1+1)
Cas64-4  80.9ns ± 0%  80.9ns ± 0%   ~     (p=1.000 n=1+1)

Fixes #57344

Change-Id: I190a7fc648023b15fa392f7fdda5ac18c1561bac
Reviewed-on: https://go-review.googlesource.com/c/go/+/457135
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Wayne Zuo <wdvxdr@golangcn.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/458357
Run-TryBot: David Chase <drchase@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
@dmitshur dmitshur added arch-riscv Issues solely affecting the riscv64 architecture. arch-mips labels Dec 28, 2022
@dmitshur
Copy link
Contributor

Closed by merging commit eeaf508 (CL 458357) to release-branch.go1.18.

@golang golang locked and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-mips arch-riscv Issues solely affecting the riscv64 architecture. CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Milestone
Go1.18.10
Development

No branches or pull requests
4 participants
@xen0n @dmitshur @dr2chase @gopherbot