-
Notifications
You must be signed in to change notification settings - Fork 11
cgo/clapack: revise clapack API #137
Conversation
This reduces allocations and harmonises the cgo and native behaviours.
ping |
my $typeElem = substr $goType, 2; | ||
my $bp = << "EOH"; | ||
var _$var *$typeElem | ||
if len($var) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the result and rethinking it again I'm not sure anymore if we should do this, and much less for all slice parameters. I triggered this because of Dgehrd and its tau slice parameter that is not needed for small (corner-case) values of n. There we panicked unnecessarily. But on the other hand, for example in Dgehrd work
must always have length at least 1, its first element is always written to. If work has zero length, we unnecessarily postpone the panic/segfault to inside the C.LAPACKE call.
Dealing with this selectively is too much work and hard to automatize for code generation. It's not ideal but keeping the current status is probably better for the moment? The behavior of the calls via the clapack
package will not be 100% equivalent to the underlying implementation (for corner-case or incorrect arguments) but it may not matter much since in the vast majority of cases it will be us doing these calls anyway from the cgo
package.
Of course, this does not include the switch to _work
calls, I do support doing that.
It's not ideal, but I don't have a huge problem with this.
If we know that it's just
If you want and think the |
I don't know if it can be said this way. I noticed
Based on the above and on panicking inside lapacke not being a problem, your previous, more benevolent approach seems better? I'm sorry about this back and forth, I'm still sorting my thoughts out about this. |
@btracey what do you think here? |
I'm not sure that it's more benevolent and I don't think that throwing in C is not a problem (just that I don't have a huge problem with it under the circumstances where clapack is essentially not for public consumption and is not intended to be used without prior checks). |
A more careful approach than |
My gut reaction is that allowing work to be supplied is nice. It aligns the interfaces more, and hopefully will reduce bugs. My second gut reaction is that we don't need lots of checks on clapack. There's the first simple level which we could probably fix in autogeneration (nil checks, zero length), but there are tons of other checks we don't want to "autogenerate" by hand. We have mat64 for most cases, and cgo for some corner cases. If one really needs clapack, you're going to have to do your own validation anyway. |
Sorry, @btracey can you clarify? The options here are to merge the first two commits, the first four, or all (the last is not actually an option based on what @vladimir-ch has said about taup/tauq) - i.e. there is no nil checking here.
|
Sorry, I should have been more clear. It seems like we can just do the first two. It helps the code be more efficient, and it helps reduce bugs with a more cohesive interface. The other checks increase correctness, but sacrifice speed and clarity. I'm not that concerned about the speed, but it seems like clapack is best served as a very small shell around lapack. I'd rather keep the safety checks we need in cgo (and native). I'm not sure that I'm right. Additional safety seems nice, but my point earlier was that many checks are necessary in order to actually make the clapack call safe. For example, the input slice may have length > 0, but not sufficiently long and so the c code has a segfault. If someone really wants a safe call, they need their own list of checks, and so the ones in this autogeneration will be redundant. I suppose this allows nil arguments... I don't have that good a sense of the tradeoffs. It probably doesn't matter that much despite the length of this comment. The c overhead is likely longer than these checks, so hopefully the additional time is negligible. |
The commits that mutate zero-length slices to be passed as nil pointers are not for safety, they are there to allow zero length slices to be passed - as was a requirement for |
Yea. I had been thinking that cgo should just do the checks for the zero-length slices and return early. This is the wrong mentality. clapack should allow any legal lapack call, and so it should have the checks that you include here. I would say all of them and not just tau, as lapack is inconsistent on which allow zero-length arguments and which do not. |
OK. That's the first four commits. I think we all agree with this approach. I'll wait till tonight for any final dissent, and then merge (fixing dgehrd if necessary). |
Yes, I agree, all of them, not just tau. |
Thanks for being patient with the discussion |
This is the easiest 20kSLOC I've ever written. |
This set of changes brings the
work
,iwork
andlwork
parameters to all clapack functions that use them - rather than allowing LAPACKE to do allocations behind the scenes. This harmonises the behaviour of cgo and native calls, with the exception of the range ofiwork
elements which must fit inint32
in the cgo calls due to the backing types in the C/Fortran.We also now pass
nil
instead of panicking when a zero-length slice is passed by the user. This means that it is the responsibility of the clapack caller to ensure appropriate length checks are made to ensure a meaningful error when they are wrong; for the majority of cases this will be us in cgo.@btracey @vladimir-ch Please take a look.