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

Make testing_roc_alloc align the allocation #7287

Open
MatthewJohnHeath opened this issue Dec 1, 2024 · 10 comments
Open

Make testing_roc_alloc align the allocation #7287

MatthewJohnHeath opened this issue Dec 1, 2024 · 10 comments

Comments

@MatthewJohnHeath
Copy link
Contributor

Currently testing_roc_allocin crates/compiler/builtins/bitcode/src/utils.zig ignores the value passed as alignment and always returns a pointer to memory with alignment 1. This caused me a few hours of confusion trying to test an allocation that needed aligning.
Ideally, it would align to whatever value is passed (up to that supported by Zig), but allocating with an alignment not known at compile time seems hard (?).
Always aligning to 16 and panicking if a higher alignment is requested seems like it would cover all likely cases (without wasting too many bytes)

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 2, 2024

@lukewilliamboswell
Copy link
Collaborator

@MatthewJohnHeath are you planning on implementing this?

@MatthewJohnHeath
Copy link
Contributor Author

@lukewilliamboswell yes,. I don't think I have permissions for making a pull request to the real repo, but I'll try to the changes on a branch of my forked version today.

@MatthewJohnHeath
Copy link
Contributor Author

Oh... this does not work quite how I thought it did though. It will take longer

@MatthewJohnHeath
Copy link
Contributor Author

Oh it actually is pretty simple, the pointer just needed to be passed to the dealloc as a type with the same alignment : Here is the branch https://github.com/MatthewJohnHeath/rocSHA/tree/7287-Make-testing_roc_alloc-align-the-allocation

@MatthewJohnHeath
Copy link
Contributor Author

MatthewJohnHeath commented Dec 6, 2024

Technically it is aligning as whatever @ alignOf(usize) is. Making it 8 on arbitrary hardware is probably possible, but it's trickier and there isn't an obvious benefit. EDIT; @ alignOfwithout the space actually ats someones account.

@MatthewJohnHeath
Copy link
Contributor Author

OH wait again! It was meant to be to 16!

@MatthewJohnHeath
Copy link
Contributor Author

OK this is the PR (The git hub instructions to link the issue and the PR are not working for me)

@JRI98
Copy link
Contributor

JRI98 commented Dec 13, 2024

Fixed by #7312?

@MatthewJohnHeath
Copy link
Contributor Author

It should be, but I think I stopped a mistake in my code that was merged there....

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

No branches or pull requests

4 participants