-
Notifications
You must be signed in to change notification settings - Fork 262
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
Fix issues on 32-bit platforms #354
Conversation
Prepare for using narrower SuiteSparse_long together with 32-bit indices.
I can't move back to SuiteSparse_long, for many reasons. So I can't do this PR. I need to be able to use either 32 bit or 64 bit indices regardless of the platform. I'll need to fix the 32-bit problem another way. I'm at a conference all next week but will be working on it after that. |
What is the reason why SuiteSparse_long needs to be 64-bit regardless of the platform? |
As far as I'm aware GraphBLAS works fine on 32-bit machines (despite using int64 for indexing). This is the preferred behavior and why I imagine Tim wants both to work. |
Many reasons, including Julia and others. SuiteSparse_long is very awkward to maintain, as well. It has a size that can vary by platform, and that was never the intent for this type. I took out SuiteSparse_long and replaced it with int64_t on purpose and I don't want to go backwards. |
I might be missing something here: But what is the point in using a type for indexing that is larger than Additionally, What kind of issues is Julia experiencing with the former definition (that was used for years)? Imho, changing |
With the changes proposed here, there won't be different definitions for Edit: The main point why I chose to switch back to using the |
It would be very nice if someone could take the time to lay out what the reasons to switch to integer types that are larger than pointer types was. (Ideally, that would be something more than "because I wanted to". Albeit, that can also be a valid reason.) |
The main issue is the unknown range of the index. The SuiteSparse_long was always meant to be 64 bits on all platforms, and not the same size as ptrdiff_t. It was a mistake that I didn't realize until much later, that it was actually 32-bits on systems with a 32-bit pointer. It's important for a data type to be predictable, so overflow and max. sizes can be predictable. If the index sizes varies depending on the pointer size, then it's not predictable. In addition, there are cases where you want to create a matrix that is larger than your memory size (in that case, it would have to contain a lot of empty space, of course. In MATLAB for example, it's handy to be able to do x = sparse(n,1) where n = 2^48 (say), even though the system would not have anywhere near as much memory as 2^48 bytes or words. SPQR in particular can be used to do QR factorization on a very tall and thin matrix (n -by m where n >> m) and there would be natural cases even on a 32-bit machine where n > 2^32 but m is very small (say 10). But the main reason is predictability, where the integer I'm working with (other that sizeof (void *), etc) can have a known size so I know how to deal with integer overflow issues when computing with those integers. |
This PR is an attempt to fix the regression since SuiteSparse 6 on 32-bit platforms which was caused by switching the indexing type from
long
(32-bit wide on 32-bit platforms) toint64_t
(64-bit wide on 32-bit platforms).This was a lot harder than necessary because
int64_t
was hardcoded all over the different libraries.The first couple of changes are mainly to revert to using
SuiteSparse_long
as the (larger) indexing type. That identifier is well established in downstream projects and never stopped being defined.Using it allow to more easily change the indexing type for all affected libraries in one central location (now and in the future).
The actual switch to
intptr_t
is done in one of the last commits.These changes fix the segmentation fault when using the
_l_
functions on 32-bit at least for me on Windows. But I expect that it will also fix those crashes on other 32-bit platforms.Fixes #221.