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

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

Closed
gopherbot opened this issue Dec 15, 2022 · 2 comments
Labels
arch-loong64 Issues solely affecting the loongson architecture. 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

Comments

@gopherbot
Copy link

@dr2chase requested issue #57282 to be considered for backport to the next 1.19 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 added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 15, 2022
@gopherbot gopherbot added this to the Go1.19.5 milestone Dec 15, 2022
@gopherbot
Copy link
Author

Change https://go.dev/cl/458355 mentions this issue: [release-branch.go1.19] cmd/compile: sign-extend the 2nd argument of the LoweredAtomicCas32 on loong64,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
Copy link
Author

Closed by merging c176029 to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Dec 28, 2022
…the LoweredAtomicCas32 on loong64,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 #57345

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/+/458355
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 arch-loong64 Issues solely affecting the loongson architecture. labels Dec 28, 2022
@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.
Labels
arch-loong64 Issues solely affecting the loongson architecture. 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
Development

No branches or pull requests

3 participants