Skip to content
This repository was archived by the owner on Nov 24, 2018. It is now read-only.

Conversation

@kortschak
Copy link
Member

This is a clean up of the code generation (reducing the line count
sigificantly and making failure much noisier prior to the CGO call -
though will only catch the most egregious errors).

The approach I've taken here differs significantly from that in blas/cgo
where all the checks are done in the generated binding code. Here there
is too much complexity, so we do the checks in the Implementation method
and then call the clapack function.

@btracey

@kortschak kortschak force-pushed the clapack branch 7 times, most recently from d7ad18d to bcdd301 Compare July 15, 2015 03:47
@btracey
Copy link
Member

btracey commented Jul 15, 2015

I don't fully understand. This sounds blunt, but it's not intended that way. Questions:

  1. Shouldn't we actually generate the clapack code like we do in blas/cgo? This way the user doesn't have to autogenerate go code, as intended.
  2. I don't understand the motivation for the two different locations. You say 'here there is too much complixity'. Do you mean diversity/irregularity in function calls? Why does that motivate the split? It seems to me that in both blas and lapack, there are a set of checks, and then there is the actual C call.
  3. Related to 2). If this approach is best for Lapack, should blas/cgo be changed in the same way?
  4. Is it worth trying to autogenerate the preamble and checks? We could have a "//gonum:startcopy" style comment. We then have a wrapper that reads from the start of "//gonum:startcopy" in each file of lapack/native, and generates the function prototype and all of the comments and checks. So, for Dpotrf, it would be something like
//gonum:startlapackcopy

// Dpotrf computes the cholesky decomposition of the symmetric positive definite
// matrix a. If ul == blas.Upper, then a is stored as an upper-triangular matrix,
// and a = U U^T is stored in place into a. If ul == blas.Lower, then a = L L^T
// is computed and stored in-place into a. If a is not positive definite, false
// is returned. This is the blocked version of the algorithm.
func (impl Implementation) Dpotrf(ul blas.Uplo, n int, a []float64, lda int) (ok bool) {
    bi := blas64.Implementation()
    if ul != blas.Upper && ul != blas.Lower {
        panic(badUplo)
    }
    if n < 0 {
        panic(nLT0)
    }
    if lda < n {
        panic(badLdA)
    }
    if n == 0 {
        return true
    }
//gonum:endlapackcopy
    nb := impl.Ilaenv(1, "DPOTRF", string(ul), n, -1, -1, -1)
        // Other code

There is still the complexity of mapping the Go signature to the C signature, but it forces the comments, signatures, and checks to be in sync.

@kortschak
Copy link
Member Author

Bluntness not noticed.

  1. We do generate the bindings - previously we didn't. Now things just work if the CGO_LDFLAGS var is correctly set.
  2. If I put all the generated code into cgo we have a documentation page that has ~900 func entries, virtually all of which are not things we support via the API. Putting the generated code into cgo/clapack is essentially equivalent to the syscall package with the lapack/cgo package being then the equivalent of the os package. The main difference between the blas/cgo code generation and the code generation here is how the parameters and returns are handled. @dane-unltd made what I think is quite a nice decision to treat the pointer parameters for fortran outs as slices - this gives us the same semantics ans significantly simplifies the code generation logic (that there is no special cases here like in blas/cgo is a testament to that). The unfortunatel consequence of this is that the calls are all unsafe. Figuring out all the cases based on the irregularish parameter naming in order to get the bounds checks done automatically would be a nightmare. So I don't bother at all in the functions, but as we add methods to cgo.Implementation we can do that by hand.
  3. No, I think the difference in size and regularity of the header files is such that it works for blas. If LAPACK were more regular, I might put in the effort to get it to work here.
  4. Maybe, but I don't think it's worth it at this stage - bearing in mind that the cgo.Implementation will only ever satisfy the lapack.Float64 and lapack.Complex128 (and the single precision versions), while the native will satisfy the interface of all the required subsidiary functions as well. We can consider it for later though.

@btracey
Copy link
Member

btracey commented Jul 15, 2015

  1. Sorry. I was looking for a long file (which it is), but it was hidden due to its length.
  2. Thanks for the explanation. Makes perfect sense.

@btracey
Copy link
Member

btracey commented Jul 15, 2015

I can't review the perl, but LGTM.

@kortschak
Copy link
Member Author

I've made some changes - at odds with some of the safety, though I can fix that, but more informative.
PTAL

@btracey
Copy link
Member

btracey commented Jul 15, 2015

Could you explain the distinction between (*C.double)(&a[0]) and unsafe.Pointer(&a[0])? They're basically equivalent as far as C is concerned, right?

Could you give an example of the code change for what you'd like to comment on. It would be hard for me to understand the diff without an example, and unfortunately the cgo file is too big for the github diff to work.

@kortschak
Copy link
Member Author

kortschak commented Jul 15, 2015 via email

@kortschak
Copy link
Member Author

I've decided it's too irregular to do now, for example tau is a []float64 or a *float64 depending on the function. The only way to do it here is to have a table to labels and types - how we should do blas/cgo as well in the long run.

If we say that cgo/lapack is not subject to any API stability then I think we are fine.

@kortschak
Copy link
Member Author

Merging on the basis of your previous LGTM which is now the current state.

This is a clean up of the code generation (reducing the line count
sigificantly and making failure much noisier prior to the CGO call -
though will only catch the most egregious errors).

The approach I've taken here differs significantly from that in blas/cgo
where all the checks are done in the generated binding code. Here there
is too much complexity, so we do the checks in the Implementation method
and then call the clapack function.

Also use CGO_LDFLAGS env var; this is how we do things in blas and it
allows us to have code that will work with go get when the environment
is correctly set up - without requiring go generate or perl use by the
client.
@kortschak kortschak merged commit 434c403 into master Jul 16, 2015
@kortschak kortschak deleted the clapack branch July 16, 2015 00:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants