-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove C scalars #70
Comments
I like this a lot. What, however, is the perf impact on SuiteSparse right now? How expensive is the boxing? Lots of reasons to do this though. |
I think the performance impact on SuiteSparse and LAGraph will be minimal.
The "boxing" of the plain C scalar into a GrB_Scalar is very cheap.
Code complexity of the plain C scalars is a nightmare. See my _Generic
macro for GrB_apply, for example. There are NINETY TWO versions of GrB_apply, if I include my GxB_FC* complex types,
or EIGHTY if I exclude them and just count the GrB methods.
All of those must be wrapped into the single GrB_apply macro. Ack.
Try (!) to understand my #define GrB_apply in GraphBLAS.h.
Using GrB_Scalars also might improve future non-blocking exploits.
|
If we do this, there should also be a way to get the identity from a monoid. Reducing an empty Vector or Matrix behaves differently whether the result is a C scalar (is monoid identity) or a GrB scalar (is empty). |
I think that’s a good idea I’m any case. I have a GxB method that already
does that.
|
The problem of name explosion gets much, much worse, if GxB_eWiseUnion gets added to the spec. That function has 2 input scalars. They can be different types, so that would be 13^2 = 169 entries for built-in C types, the user void, and the GrB_Scalar, or 15^2 = 225 if I include my two complex types. Then there is a Matrix version and Vector version. Total # of function names with just GrB: 2 * 169 = 338. That's insane. Instead of this, I just have two functions that take in two GrB_Scalars: GxB_Vector_eWiseUnion and GxB_Matrix_eWiseUnion. |
@DrTimothyAldenDavis what are your thoughts on getElement and setElement? Somewhat related: could/should we have e.g. |
I think set and get Element should remain the same.
Getting nvals could require computation so it could use a descriptor. I’m
ambivalent about returning a GrB scalar instead of a uint64. I see pros and
cons. Maybe have two functions?
|
Here's a crazy example of how insane GrB_apply is, because of all the C scalars in the spec. This file (call it ugly.c) won't run, of course, because the matrices are defined, but just look at the C preprocessor output:
Seems tame, right? OK, now look at the output of "cc -E ugly.c" ... where the GrB_apply macro expands to all all 90 variants:
Somebody had to write that nasty macro: which also uses a helper macro that appears in many places: It's doable but really insane, and this kind of code (required by the spec) is a barrier to the creation of other GraphBLAS implementations. |
With the introduction of GrB_Scalar, do we still need C scalars for most functions? Tim D. would like to remove them for everything except getElement and setElement. Doing so will remove lots of complex code which doesn't ultimately do anything useful. It just adds complexity.
Removing these would lower the barrier for any alternate C implementation of the spec.
The text was updated successfully, but these errors were encountered: