-
Notifications
You must be signed in to change notification settings - Fork 58
-
Description
Arrayfire::row, arrayfire::col uses u64 to index arrays
index_gen uses f32, f64, u32, u64 to index arrays
arrayfire::sparse uses i32 to index arrays
It is a complete mess of indexing. It is easier to change all indexing to just u64 because negative values might cause errors.
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 3 comments
-
I am not sure if that assessment is correct and here are my reasons:
rowandcolorslicethey are not indices to elements, they are indices to a particular row or column. These values are converted to af64before it is passed down to ArrayFire's C FFI. I choseu64but it can beusizetoo. However, I choseu64to reflect the bit size of type used by ArrayFire for dimension/axis size. Note that the type of value that points to a particular row/column/slice is not same as index to a particular element of Array.arrayfire:sparse- this is currently limitation of how sparse Array's are implemented internally in ArrayFire, unless that changes this can't be changed. Otherwise we would end up doing unnecessary up casts and down casts in rust wrapper.index_gen- We can't force users to always useu64indices. On top of that, 64 bit types have additional performance penalty which can be avoided if a given use case's index range can be represented by 32 bit just fine.
Among the three mentioned, I think the only candidate for converting the indices type to u64 is sparse creation. However, that needs to be addressed in arrayfire repository and it would involve considering performance implications of using u64 and if upstream dependencies like cuSparse support for u64 indices etc.
Beta Was this translation helpful? Give feedback.
All reactions
-
There are floating point, signed integer, unsigned integer, and complex types.
Using the process of elimination, floating point types are bad for indexing because they can have negative values (-1,-2,-3) and they can have non integer values (1.1,2.4,5.3). Therefore, floating point can cause many errors and should not be used for indexing.
Complex data types are bad for indexing because they have the same disadvantages as floating point. They should not be used for indexing.
For signed integer types, they can have negative values (-1,-2,-3) which can cause the index to go out of the array's memory region and crash GPUs and CPUs. They should not be used for indexing.
For the unsigned integer types, there are u8,u16,u32,u64, and usize.
u8 is bad because you can only access 256 elements.
u16 is bad because you can only access 65536 elements.
u32 is bad because you can only access 4294967296 elements. Assuming you have RTX 3090 (24 GB RAM) and you are using a 32 bit array, at most you can only access 17.18 GB out of the 24 GB. That means you can't use all of your GPU.
usize is bad because it depends on the ISA of the machine. If you write your program and debug on AMD machine with an x86_64 ISA, then usize will be u64 and it can access 1.8446744e+19 elements. If you recompile it on NVidia jetson with ARM 32 bit ISA, the program's usize will be u32 and it can't access anything above 4294967296 elements. This will introduce indexing errors in your program. This defeats arrayfire's "Code once, run anywhere!".
By the process of elimination, u64 is the only data type that doesn't cause run-time errors in indexing. The functions can be rewritten to only accept u64 and can be internally casted to any other datatype.
Beta Was this translation helpful? Give feedback.
All reactions
-
I think I wasn't clear and/or elaborate enough earlier.
- I can say for certain that we don't allow complex type arrays as indexing arrays. However, at the moment rust wrapper isn't limiting that using any trait bound, this can be definitely improved. Note that, if the user uses indexing with complex types, upstream does throw error. So, there is a check in place - just not a compile time check at the wrapper level, yet. Hence, I have raised an issue to track that addition here Add trait bounds for indexing to allow valid types that match upstream Checks #256
- About negative indices, I would recommend you going through the upstream project's indexing tutorial to understand what happens when negative indices are given - http://arrayfire.org/docs/indexing.htm. I should add this information about negative indices to rust wrapper indexing tutorial too - https://arrayfire.org/arrayfire-rust/book/indexing.html Thanks for pointing that out. Raised an issue for this Mention about negative indices in rust indexing tutorial #257
- usize is not good I agree, it is not used in the wrapper already.
- About all the other bit sizes, floating point or not, note that these are provided for ease of use in user's code. Internally, they are automatically up-casted to a 32 bit integer (down-casted in the case of 64 bit right now, I think this needs to be fixed upstream) before the actual indexing operation takes place. I shall discuss the case of 64 bit array indexing being cast to 32bit with the team.
- About your suggestion moving from 32 bit to 64 bit, there is no single silver-bullet type that can work for all cases. Memory is not the only parameter. With scientific computing algorithms, each use case is different and using different types has different performance effect, it is not a zero cost switch to 64 bit type for indexing. We think this has to be a deliberate choice from the programmer based on their use case. We cannot force such type restriction which has a significant performance impact.
Beta Was this translation helpful? Give feedback.