gonum / internal

Internal routines for the gonum project [DEPRECATED]
21 stars 9 forks source link

Added assembly routine and stub for ddot #1

Closed btracey closed 9 years ago

btracey commented 9 years ago

PTAL @fhs @jonlawlor @kortschak

btracey commented 9 years ago

I have tested this locally with floats and it works.

fhs commented 9 years ago

Do we really want to split DdotUnitary from DdotInc? I thought all the assembly would be in this internal package.

In the future, if AVX support is added, there will be an init function that detects if AVX is supported by the CPU and for example set the function DdotUnitary to DdotUnitaryAVX. Otherwise it'll set DdotUnitary to DdotUnitarySSE2. This logic would need to be repeated in internal/asm, blas/native and floats package.

btracey commented 9 years ago

I'd defer to your opinion on the subject. My thought was to minimize the code in internal and only put code there that is used by multiple packages. If you think it's better to have them together, then okay.

That would be done at runtime instead of compile time? What you say is not because of the internal package, but is just a fact, right? I guess I assumed such a thing would be done with a build tag. Is there a way we could make that only happen in one place (for example, internal)? Sorry, this is something I don't have experience with.

fhs commented 9 years ago

It would need to happen in runtime if you want "go get" to work as it normally does, without burdening the user to specify the right build tag. I think Go does something similar for ARM runtime. It should be quick -- only happens once when you import the package. The issue is unrelated to internal package. If all the assembly is in this internal package, it'd only need to happen here.

Besides this point, I think DdotUnitary and DdotInc are related functions that belong in the same place. It's cleaner.

btracey commented 9 years ago

Added.

Why does the code here (the same as in blas/native) have the extra file with the +build tag? Looking at the implementation of asin (http://golang.org/src/math/asin.go), the non-assembly implementation is just the unexported version of the function, and the assembly files are just there. Is there something different about our function that requires the current behavior, or should we change this to match that convention?

fhs commented 9 years ago

The math package have stubs for all the architectures (e.g. http://golang.org/src/math/asin_arm.s). I just didn't bother creating all the stubs. There's also a risk of Go adding support for a new architecture (e.g. ppc64) but us not having the stubs for it. It's safer the way it is now -- the pure Go version is the default but we turn on the assembly version for selected architectures.

fhs commented 9 years ago

LGTM

btracey commented 9 years ago

What do the unexported functions (in math) do then? I read https://groups.google.com/forum/#!topic/golang-nuts/rlGvw9hVhoY , and like the OP had assumed the unexported functions were for fallback. I guess that's false, but I don't understand what they do do instead. Ian says: "The ones without an architecture-specific implementation simply jump to foo". He points to sin, but it looks like sin does have an architecture-specific implementation (all of the sin_*.s functions), so under what cases would it jump to foo?

fhs commented 9 years ago

Read the assembly for example here: http://golang.org/src/math/sin_amd64.s

The JMP ·sin(SB) basically means call the Go sin function (i.e. even though there is a Sin function in the .s file for amd64, the assembly function just calls the Go code!)

btracey commented 9 years ago

Oh! I understand now. Thank you.