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

Malloc with varsize (option 1) implementation #1082

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wrrobin
Copy link
Collaborator

@wrrobin wrrobin commented Feb 3, 2023

This is a test PR for malloc varsize implementation with the following syntax:

void *shmem_malloc_varsize (size_t size, size_t global_max_size);
void *shmem_calloc_varsize (size_t count, size_t global_size, size_t global_max_count);

@wrrobin
Copy link
Collaborator Author

wrrobin commented Feb 3, 2023

@davidozog @stewartl318 - Does this implementation look correct to you?
I still need to check if a subsequent symmetric malloc works without any issues.

@stewartl318
Copy link
Collaborator

The implementation looks right, but I don't like calloc_varsize. I though we decided that size should be the same for all PEs, and global_max_count is the same for all PEs, but that my_count could be different for each PE.
I can't imagine any program that would have a different data size on different PEs.

@wrrobin
Copy link
Collaborator Author

wrrobin commented Feb 4, 2023

@stewartl318 - thanks for your comments. You do have to pass size argument even though it is fixed for all PEs. Otherwise, for example, how would you know if you are allocating for int or for long? This actually brings up a question as well.. we do not have checks for regular malloc/calloc that the sizes for all PEs are same. But, I think that is up to the user anyway. @davidozog - thoughts?

One update is, I do see issues with this code for the subsequent symmetric allocations. I think the reason is dlmalloc internally uses padding depending on the allocation size requested based on their algorithm. I will see how to fix it.

@stewartl318
Copy link
Collaborator

I was imprecise.
The proposed function is
SHMEM_FUNCTION_ATTRIBUTES void* SHPRE()shmemx_calloc_varsize(size_t my_count, size_t my_size, size_t global_max_count, size_t global_max_size);

My point is that all PEs should have my_size == global_max_size. I cannot think of a counterexample. Therefore the function does not need both.

Does shmem_calloc have a rule that size and count have to be the same on all PEs? Or is the rule that size*count must be the same on all PEs?

@wrrobin
Copy link
Collaborator Author

wrrobin commented Feb 6, 2023

@stewartl318 - By any chance, are you looking at the first commit only? Please check the latest commit or overall changes. The first commit was done some time back, but I have updated that before opening this PR.

@stewartl318
Copy link
Collaborator

Blush. Yes Wasi, I was not looking at the latest version. I am ok with the newest one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants