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.

Sign up for GitHub

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

Update lapacke related C type usage #182

Merged
merged 1 commit into from
Oct 22, 2023
+17 −19
Merged

Update lapacke related C type usage #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions la/matrix_ops.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ pub fn matrix_svd(mut s []f64, mut u Matrix[f64], mut vt Matrix[f64], mut a Matr
if copy_a {
acpy = a.clone()
}
vlas.dgesvd(byte(`A`), byte(`A`), a.m, a.n, acpy.data, 1, s, u.data, a.m, vt.data,
a.n, superb)
vlas.dgesvd(&char('A'.str), &char('A'.str), a.m, a.n, acpy.data, 1, s, u.data, a.m,
vt.data, a.n, superb)
}

// matrix_inv computes the inverse of a general matrix (square or not). It also computes the
Expand Down
18 changes: 8 additions & 10 deletions vlas/conversions.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,16 @@ pub fn c_uplo(up bool) vblas.Uplo {
return .lower
}

pub fn l_uplo(up bool) u8 {
if up {
return `U`
}
return `L`
fn l_uplo(up bool) u8 {
return if up { `U` } else { `L` }
}

pub fn job_vlr(do_calc bool) u8 {
if do_calc {
return `V`
}
return `N`
fn job_vlr(do_calc bool) &char {
return &char(if do_calc {
'V'
} else {
'N'
}.str)
}

// slice_to_col_major converts nested slice into an array representing a col-major matrix
Expand Down
14 changes: 7 additions & 7 deletions vlas/lapack.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,21 @@ import vsl.vlas.internal.vblas

fn C.LAPACKE_dgesv(matrix_layout vblas.MemoryLayout, n int, nrhs int, a &f64, lda int, ipiv &int, b &f64, ldb int) int

fn C.LAPACKE_dgesvd(matrix_layout vblas.MemoryLayout, jobu byte, jobvt byte, m int, n int, a &f64, lda int, s &f64, u &f64, ldu int, vt &f64, ldvt int, superb &f64) int
fn C.LAPACKE_dgesvd(matrix_layout vblas.MemoryLayout, jobu &char, jobvt &char, m int, n int, a &f64, lda int, s &f64, u &f64, ldu int, vt &f64, ldvt int, superb &f64) int

fn C.LAPACKE_dgetrf(matrix_layout vblas.MemoryLayout, m int, n int, a &f64, lda int, ipiv &int) int

fn C.LAPACKE_dgetri(matrix_layout vblas.MemoryLayout, n int, a &f64, lda int, ipiv &int) int

fn C.LAPACKE_dpotrf(matrix_layout vblas.MemoryLayout, up u32, n int, a &f64, lda int) int

fn C.LAPACKE_dgeev(matrix_layout vblas.MemoryLayout, calc_vl byte, calc_vr byte, n int, a &f64, lda int, wr &f64, wi &f64, vl &f64, ldvl_ int, vr &f64, ldvr_ int) int
fn C.LAPACKE_dgeev(matrix_layout vblas.MemoryLayout, calc_vl &char, calc_vr &char, n int, a &f64, lda int, wr &f64, wi &f64, vl &f64, ldvl_ int, vr &f64, ldvr_ int) int

fn C.LAPACKE_dlange(matrix_layout vblas.MemoryLayout, norm byte, m int, n int, a &f64, lda int, work &f64) f64
fn C.LAPACKE_dlange(matrix_layout vblas.MemoryLayout, norm &char, m int, n int, a &f64, lda int, work &f64) f64

fn C.LAPACKE_dsyev(matrix_layout vblas.MemoryLayout, jobz byte, uplo byte, n int, a &f64, lda int, w &f64, work &f64, lwork int) int
fn C.LAPACKE_dsyev(matrix_layout vblas.MemoryLayout, jobz &char, uplo &char, n int, a &f64, lda int, w &f64, work &f64, lwork int) int

fn C.LAPACKE_dgebal(matrix_layout vblas.MemoryLayout, job byte, n int, a &f64, lda int, ilo int, ihi int, scale &f64) int
fn C.LAPACKE_dgebal(matrix_layout vblas.MemoryLayout, job &char, n int, a &f64, lda int, ilo int, ihi int, scale &f64) int

fn C.LAPACKE_dgehrd(matrix_layout vblas.MemoryLayout, n int, ilo int, ihi int, a &f64, lda int, tau &f64, work &f64, lwork int) int

Expand Down Expand Up @@ -76,7 +76,7 @@ pub fn dgesv(n int, nrhs int, mut a []f64, lda int, ipiv []int, mut b []f64, ldb
// Note that the routine returns V**T, not V.
//
// NOTE: matrix 'a' will be modified
pub fn dgesvd(jobu byte, jobvt byte, m int, n int, a []f64, lda int, s []f64, u []f64, ldu int, vt []f64, ldvt int, superb []f64) {
pub fn dgesvd(jobu &char, jobvt &char, m int, n int, a []f64, lda int, s []f64, u []f64, ldu int, vt []f64, ldvt int, superb []f64) {
info := C.LAPACKE_dgesvd(.row_major, jobu, jobvt, m, n, &a[0], lda, &s[0], &u[0],
ldu, &vt[0], ldvt, &superb[0])
if info != 0 {
Expand Down Expand Up @@ -199,6 +199,6 @@ pub fn dgeev(calc_vl bool, calc_vr bool, n int, mut a []f64, lda int, wr []f64,
}
}

pub fn dlange(norm byte, m int, n int, a []f64, lda int, work []f64) f64 {
pub fn dlange(norm &char, m int, n int, a []f64, lda int, work []f64) f64 {
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you just use str instead of &char? I think V handles that when doing interop with C but I'm not totally sure

Copy link
Member

Choose a reason for hiding this comment

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

that should simplify the code because we can use str everywhere instead of casting to &char

Copy link
Member Author

@ttytm ttytm Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. The cast to char should be handled internally. The below would work:

pub fn dlange(norm string, m int, n int, a []f64, lda int, work []f64) f64 {
	return unsafe { C.LAPACKE_dlange(.row_major, &char(norm.str), m, n, &a[0], lda, &work[0]) }

The cast needs to be explicit. Just norm.str only compiles without -cstrict. Maybe this changes when the V compiler evolves. Until then &char(norm.str) is the proper usage, which is okay for internal code I guess. (I'm gonna open an issue at V regarding this, as I need to use this cast a lot in my own code bases as well when interoping with C. Just .str would add some convenience).

Regarding the norm type, it depends if string is the best. If norms are set by a single character rune would be better. I'm not too familiar with the usage of lapacke yet and can't tell if an enum is appropriate for the norms. In any case, the cast to char could and should be handled internally.

Which one should it be?

Copy link
Member

Choose a reason for hiding this comment

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

OK, makes sense! Will Merge now!

Copy link
Member Author

@ttytm ttytm Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @ulises-jeremias !

I think t was a valid point and that it's better to use &char only in the C functions.
That's why I thought it might be improved and ended the comment with a question.

So for pub fn dlange I'm changing it to rune in #184. Than it's the same usage as before for this public function. Just with internal C type handling corrected.

Regarding .str cast situation, opened an issue with a minimal repro: vlang/v#19623

🙂:+1

return unsafe { C.LAPACKE_dlange(.row_major, norm, m, n, &a[0], lda, &work[0]) }
}
Loading