miopenGetRNNTrainingReserveSize is not rounded to a convenient boundary

[amberhassaan]

Test code is having to round up the reserve space size obtained from miopenGetRNNWorkspaceSize to the next multiple of 4. This should be done inside the library call. See for example test/gru_common.hpp and search for the above api call. This is also applicable to lstm_common.hpp and rnn_vanilla_common.hpp.

CC: @JehandadKhan , @junliume

[shurale-nkn]

No, miopenGetRNNTrainingReserveSize returns number of bytes not floats, it may not be a multiple of 4. This is problem of testing file to correctly match API limitations.

[amberhassaan]

I'm asking us to round up to a multiple of 4 or 8 to be safe so that it's not needed in the tests that use float (and perhaps double if we ever choose to support it). I think it's common sense. @JehandadKhan : what do you think?

[shurale-nkn]

This is not correct to work with this buffer as float or double. It contains not only one type. Bigger allocation is not more safe, it will just miss out-of-bounds array errors more often.

[amberhassaan]

I don't understand. The code (for example test/gru_common.hpp) is setting the RNN descriptor data type as float then the reserve space size should be a multiple of sizeof(float). Otherwise, I'd think it's a bug. What's the use if I have to round it up in the test code?

[shurale-nkn]

This is incorrect allocation in GRU test.

[shurale-nkn]

the authors decided to allocate float to simplify their work, but this does not mean that this is the correct solution.

[amberhassaan]

That little rounding up seems crucial because the tests failed when I took it out. Regardless, the library should return a workspace size that is a multiple of 8 (or 4 at least). The expectation is that the user will allocate memory for that size and almost all allocators already align allocations to 8 byte boundary at least.

[shurale-nkn]

nobody expects that. This is not correct, user can make bigger allocation, but this is not expected or required.

[amberhassaan]

I disagree but let's leave it at that.

[JehandadKhan]

@shurale-nkn Is the size returned from the API usable directly or does it need to updated for the resulting buffer to be correct ?

[shurale-nkn]

@JehandadKhan User can directly use byte size returned from API. This is a correct and valid value, it does not require modification.

[amberhassaan]

@JehandadKhan : here's one of many examples where the user code doesn't work if we take out this "round up to next multiple of sizeof(T)" : https://github.com/ROCmSoftwarePlatform/MIOpen/blob/60cf9c095356a58d9ef369856b585ac1f4d39946/test/lstm_common.hpp#L918

[shurale-nkn]

@amberhassaan This is not example how it doesn't work. This is example how it used in our test, this does not mean that the user cannot use this value directly or computation fails without modification. Please do not mislead others.

In PR#2493 you can discover how the test and the user can perfectly use this value without any modifications and everything works correctly.

[amberhassaan]

All I know is that 1) our tests fail when the size is not rounded up 2) I fail to understand how we return a size that's not multiple of floats when we work with floats.

[CAHEK7]

As far as I can see 1) the test should be fixed and 2) std::vector for the workspace must not be T, it's just a memory holder and it's not even used anywhere else (and must not be used).
So if the vector will be std::byte the problem will vanish.

[CAHEK7]

@amberhassaan
The last argument is that - what do you think about the case when malloc suddenly started to demand size to be, for example, 8 bytes aligned. The arguments are the same - malloc internally allocated 8byte chunks, so why would we requested less then 8?
What shall we do if at some point such malloc function decided to allocate 256bytes chunks since it's more efficient?

That's exactly the case for this ticket - the library which allocates the memory (test wrapper) started to demand specific allocation size from the user (MIOpen algorithm) who knows exact amount of memory what he needs and don't care about any sort of internals of allocation routines.