Skip to content

cmd/compile: regression on ppc64le bit operations #73153

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

Open
randall77 opened this issue Apr 3, 2025 · 3 comments
Open

cmd/compile: regression on ppc64le bit operations #73153

randall77 opened this issue Apr 3, 2025 · 3 comments
Labels
arch-ppc64x compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@randall77
Copy link
Contributor

package main

import (
	"fmt"
	"unsafe"
)

type st = struct {
	F__ccgo0 uint64
}

var constreg = [1]int32{}

func SetBitFieldPtr64Int64(p uintptr, v int64, off int, mask uint64) {
	*(*uint64)(unsafe.Pointer(p)) = *(*uint64)(unsafe.Pointer(p))&^mask | uint64(v)<<off&mask
}

func f() {
	var nIndx int32
	nIndx = 0
	SetBitFieldPtr64Int64(next+0, constreg1[nIndx], 0, 0xffffff)
	SetBitFieldPtr64Int64(next+0, constreg1[nIndx], 24, 0xffffff000000)
}

var constreg1 = [1]int64{
	0: int64(0xFEFEFEFE),
}

var bp [16]byte
var next = uintptr(unsafe.Pointer(&bp))

func main() {
	f()
	fmt.Printf("|% x|\n", unsafe.Slice((*byte)(unsafe.Pointer(next)), unsafe.Sizeof(st{})))
	if uint32(int32(int64(*(*uint64)(unsafe.Pointer(next + 0))&0xffffff>>0)<<40>>40)) != uint32(0xFFFEFEFE) {
		panic("FAIL")
	}
	if uint32(int32(int64(*(*uint64)(unsafe.Pointer(next + 0))&0xffffff000000>>24)<<40>>40)) != uint32(0xFFFEFEFE) {
		panic("FAIL")
	}
}

This should pass, but fails on ppc64le since https://go-review.googlesource.com/c/go/+/622236

@pmur @golang/ppc64

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 3, 2025
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 3, 2025
@cagedmantis cagedmantis added this to the Go1.25 milestone Apr 3, 2025
@jkrishmys
Copy link
Contributor

Hi @randall77 . Thanks for bringing this to notice. I executed with go1.23 and go 1.24.1 on Power10le, with a few printf statements added to function f () before and after setting SetBitField in the above code.

with go 1.23 I see

Before first Set: |00 00 00 00 00 00 00 00|
After first Set: |fe fe fe 00 00 00 00 00|
After Second Set: |fe fe fe fe fe fe 00 00|
|fe fe fe fe fe fe 00 00|

with go 1.24.1 I see

Before first Set: |00 00 00 00 00 00 00 00|
After first Set: |fe fe fe 00 00 00 00 00|
After Second Set: |fe fe fe fe 00 00 00 00|
|fe fe fe fe 00 00 00 00|
panic: FAIL

The discrepancy occurs after the second call to SetBitFieldPtr64Int64. Specifically, the higher bits (24-47) are not being set correctly due to memory alignment or padding issues. Will try to figure out the changes that has happened in https://go-review.googlesource.com/c/go/+/622236.

@jkrishmys
Copy link
Contributor

jkrishmys commented Apr 8, 2025

Hi @randall77, update. On PowerPC, the RLWINM (Rotate Left Word Immediate then AND with Mask) instruction operates on 32-bit words, even on a 64-bit system. So due to updates in files PPC64.rules and rewrite.go https://go-review.googlesource.com/c/go/+/622236. , mask := mergePPC64RShiftMask(m, s, 64) or mask := -1 << s & m may result in bits being set above the 32-bit range (bit positions 32 to 63). By rejecting those masks,

// Reject if mask touches bits above 31
    if mask >> 32 != 0 {
        return 0
    }

in functions mergePPC64AndSrdi and mergePPC64AndSldi immediately after calculating mask, we were able to get the above example run without failure on ppc64le after successful build.

@pmur
Copy link
Contributor

pmur commented Apr 8, 2025

That isn't accurate. The mask only wraps if mb > me, and only those cases leave junk in the upper 32 bits. That shouldn't happen when converting (andi (srdi()).

The linked rules should only narrow existing masks. I suspect a rule is eliding a 32 to 64 bit zero-extension where it shouldn't. That should be easy to find given the small reproducer. You can look at the rlwinm/rlwnm instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-ppc64x compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

6 participants