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

chore: get rid of the golang.org/x/sys/cpu #75

Open
wants to merge 1 commit into
base: query-simd
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ For the full interface see the Godoc: <https://pkg.go.dev/github.com/philippgill
- Operators (`$and`, `$or` etc.)
- Storage:
- JSON as second encoding format
- Write-ahead log (WAL) as second file format)
- Write-ahead log (WAL) as second file format
- Optional remote storage (S3, PostgreSQL, ...)
- Data types:
- Images
Expand Down
34 changes: 34 additions & 0 deletions simd/cpu_amd64.go
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the required subset of the x/sys/cpu's cpu.X86.HasAVX2 and cpu.X86.HasFMA checks? (Sorry currently don't have much time to confirm myself)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks! Checked it and commented a question below

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package simd

// cpuid is implemented in cpu_amd64.s
func cpuid(eaxArg, ecxArg uint32) (eax, ebx, ecx, edx uint32)

// xgetbv with ecx = 0 is implemented in cpu_amd64.s
func xgetbv() (eax, edx uint32)

// useSIMD reports the availability of FMA and AVX2 cpu features
func useSIMD() bool {
var (
hasAVX2 bool
hasFMA bool

isSet = func(bitpos uint, value uint32) bool {
return value&(1<<bitpos) != 0
}
)

if maxID, _, _, _ := cpuid(0, 0); maxID < 7 {
return false
}
_, _, ecx1, _ := cpuid(1, 0)

hasFMA = isSet(12, ecx1)

if hasOSXSAVE := isSet(27, ecx1); hasOSXSAVE {
eax, _ := xgetbv() // For XGETBV, OSXSAVE bit is required and sufficient.
_, ebx7, _, _ := cpuid(7, 0)
hasAVX2 = isSet(1, eax) && isSet(2, eax) && isSet(5, ebx7)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are only these checks needed? In the reference code it appears to do, for non-Darwin OS: isSet(1, eax) && isSet(2, eax) && isSet(5, eax) && isSet(6, eax) && isSet(7, eax) && isSet(5, ebx7), based on this, this and this.

}

return hasAVX2 && hasFMA
}
55 changes: 55 additions & 0 deletions simd/cpu_amd64.s
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/* This file is a copy of https://cs.opensource.google/go/x/sys/+/master:cpu/cpu_x86.s.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case their file changes in the future, it might be better to reference a specific version of it

Suggested change
/* This file is a copy of https://cs.opensource.google/go/x/sys/+/master:cpu/cpu_x86.s.
/* This file is a copy of https://cs.opensource.google/go/x/sys/+/refs/tags/v0.20.0:cpu/cpu_x86.s.

Here is its license, which only applies to the copied parts and not to the rest of chromem-go,
which is licensed under the GNU Affero General Public License.

BSD 3-clause license

Copyright (c) 2009 The Go Authors. All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are
met:

* Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
* Redistributions in binary form must reproduce the above
copyright notice, this list of conditions and the following disclaimer
in the documentation and/or other materials provided with the
distribution.
* Neither the name of Google Inc. nor the names of its
contributors may be used to endorse or promote products derived from
this software without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#include "textflag.h"
Copy link
Owner

@philippgille philippgille May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if this could lead to any constraints on users' side? Does it work when using TinyGo as compiler? (Haven't tried with current chromem-go either though)

Copy link
Contributor Author

@rinor rinor May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if this could lead to any constraints on users' side?

not really, as long as we take care of proper build tags, nothing should break or cause additional constraints (we have default fallback)

Does it work when using TinyGo as compiler?

iirc,

  • TinyGo doesn't currently enable support for AVX instructions,
  • TinyGo does not support Go's Assembler, so we should skip building those files

If we want to attempt to build TinyGo "normal" binaries we can manage the builds with the tinygo build tag so building on amd64 with TinyGo will just skip the custom code.
I will take care of that.

However I don't think tinygo works right now (unrelated to the changes being worked on here). Tried quickly to build the minimal example with TinyGo and it panics

checkId: 22 should be 23
panic: bootstrap type wrong id: mapType mapType not <nil>

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be // +build !tinygo in both this assembly file and the cpu_amd64.go one, right?


// func cpuid(eaxArg, ecxArg uint32) (eax, ebx, ecx, edx uint32)
TEXT ·cpuid(SB), NOSPLIT, $0-24
MOVL eaxArg+0(FP), AX
MOVL ecxArg+4(FP), CX
CPUID
MOVL AX, eax+8(FP)
MOVL BX, ebx+12(FP)
MOVL CX, ecx+16(FP)
MOVL DX, edx+20(FP)
RET

// func xgetbv() (eax, edx uint32)
TEXT ·xgetbv(SB),NOSPLIT,$0-8
MOVL $0, CX
XGETBV
MOVL AX, eax+0(FP)
MOVL DX, edx+4(FP)
RET
7 changes: 7 additions & 0 deletions simd/cpu_other.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//go:build !amd64

package simd

func useSIMD() bool {
return false
}
4 changes: 1 addition & 3 deletions simd/simd_amd64.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ package simd

import (
"runtime"

"golang.org/x/sys/cpu" // TODO: Can we get rid of this dependency?
)

var UseAVX2 bool = cpu.X86.HasAVX2 && cpu.X86.HasFMA && runtime.GOOS != "darwin"
var UseAVX2 bool = useSIMD() && runtime.GOOS != "darwin"