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

Fix issues on 32-bit platforms #354

Closed
wants to merge 18 commits into from

Conversation

mmuetzel
Copy link
Contributor

@mmuetzel mmuetzel commented Jul 23, 2023

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) to int64_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.

Prepare for using narrower SuiteSparse_long together with 32-bit indices.
@DrTimothyAldenDavis
Copy link
Owner

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.

@mmuetzel
Copy link
Contributor Author

What is the reason why SuiteSparse_long needs to be 64-bit regardless of the platform?
Is it for some downstream projects? If it is, would it be possible to only support platforms where intptr_t is 64-bit in those projects (instead of stopping support for other platforms in SuiteSparse proper)?

@rayegun
Copy link
Contributor

rayegun commented Jul 24, 2023

SuiteSparse_long is not desirable from the Julia perspective. Having different definitions per platform complicates things.

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.

@DrTimothyAldenDavis
Copy link
Owner

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.

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jul 24, 2023

I might be missing something here: But what is the point in using a type for indexing that is larger than sizeof(void*) on a given platform? At the very least, it is dangerous to try and do that without special care.

Additionally, int64_t is a different type on different platforms. IIUC, it is long on 64-bit Linux and long long on 32-bit Linux. That makes it awkward to share pointers to the same data between SuiteSparse and other libraries (that don't automagically build with different types in their interface).

What kind of issues is Julia experiencing with the former definition (that was used for years)?

Imho, changing SuiteSparse_long to int64_t was an odd design choice (to say the least).

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jul 24, 2023

SuiteSparse_long is not desirable from the Julia perspective. Having different definitions per platform complicates things.

With the changes proposed here, there won't be different definitions for SuiteSparse_long for different platforms. It would be intptr_t on all platforms.

Edit: The main point why I chose to switch back to using the SuiteSparse_long (and derived) identifiers was to make it easier to change the type in the future (e.g., in case future standards might add a new identifier that would match better what is needed).
Additionally, this allowed to make the changes incrementally without breaking everything in intermediate steps. (This won't be an issue in case SuiteSparse_long would be used in the sources instead of hardcoding a certain type or identifier.)

@mmuetzel
Copy link
Contributor Author

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.)

@DrTimothyAldenDavis
Copy link
Owner

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.

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