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

lapack: adapt to removal of Job and Comp types in lapack package #38

Merged
merged 4 commits into from
Sep 25, 2018

Conversation

vladimir-ch
Copy link
Member

This PR adapts to the changes in gonum/gonum#572 by using byte type instead of lapack.Comp and lapack.Job. This removes the dependence of this package on the lapack package and I think we should do the same for the blas constants. I will send a PR for this later.

Please take a look.

Without this change 'go generate' fails on my machine with:

binding: failed to parse ["lapacke.h"]:
/usr/include/bits/byteswap.h:37:10: called object is not a function or
function pointer (have '<undefined>') (and 1 more errors)
Copy link
Member

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

LGTM but please wait for at least one other because of the change to byte.

@vladimir-ch
Copy link
Member Author

I could use the dedicated types instead of byte but I would have to check whether we can map to a type based exclusively on job parameter name or whether some additional logic is necessary. In the latter case I think it is easier to just use the byte type.

@vladimir-ch
Copy link
Member Author

It is not possible to decide the type based only on the parameter name and besides we would need Job and Comp types for functions that we haven't ported yet. Given that lapacke is a very low-level package anyway, I think that byte is the right choice.

@kortschak
Copy link
Member

Yes, that's my feeling. It feels right that there's almost no support.

@btracey
Copy link
Member

btracey commented Sep 23, 2018

I'm sorry, I missed this being opened. This seems like a good idea to me.

@vladimir-ch
Copy link
Member Author

I've updated the PR to include also changes for gonum/gonum#595
Could you please take a look again?

@@ -335,6 +335,7 @@ func Declarations(paths ...string) ([]Declaration, error) {
#define __inline
#define __restrict
unsigned __builtin_bswap32 (unsigned x);
unsigned __builtin_bswap16 (unsigned x);
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why did this become necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this code generation fails on my Arch machine. Motivated by your question I searched a bit and it comes from a change in glibc 2.28 that uses this built-in function if GCC 4.8 or later is available.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

@vladimir-ch vladimir-ch merged commit 7a718cd into master Sep 25, 2018
@vladimir-ch vladimir-ch deleted the lapack/job-comp-byte branch September 28, 2018 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants