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

Add atomic arch instructions #522

Closed
wants to merge 1 commit into from
Closed

Add atomic arch instructions #522

wants to merge 1 commit into from

Conversation

XAMPPRocky
Copy link
Member

This PR adds nearly all of the SPIR-V atomic instructions, the only ones not implemented are the ones around atomic flags as we don't have a type for them at the moment.

Depends on #519

@khyperia
Copy link
Contributor

I thought we had discussed this already in #442 and the plan was to expose these through the rust stdlib, instead of custom instructions?

@khyperia
Copy link
Contributor

To be clear, if I recall correctly, the entire purpose of the arch module is intended to let us prototype high level abstractions over SPIR-V concepts without getting into compiler work. There is already a high level abstraction in place for atomics (sync::atomic), no prototyping is necessary, and so I don't want to waste work to add instructions to the arch module for sake of adding instructions to the arch module. I think our effort would be better directed towards things like fixing #442.

@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented Mar 22, 2021

I thought we had discussed this already in #442 and the plan was to expose these through the rust stdlib, instead of custom instructions?

Well there's no discussion in that thread, so if we did discuss it , it wasn't there and I don't remember it beyond "yes we should have them". I don't have a strong opinion on how we do it, but as I've read the spec more I'm not sure stdlib is complete abstraction for SPIR-V's atomics. For example there are way more memory semantics in SPIR-V than just Relaxed, Release, Acquire, AcqRel, and SeqCst. stdlib also doesn't have any concept of Scope, and there's no AtomicBool equivalent in SPIR-V. So to me it seems like we'd be better off providing our own abstraction.

@XAMPPRocky XAMPPRocky force-pushed the atomics branch 2 times, most recently from 4cdb905 to 6bce70a Compare March 22, 2021 12:09
@XAMPPRocky XAMPPRocky force-pushed the atomics branch 2 times, most recently from e814b94 to 1473e20 Compare May 3, 2021 12:34
@XAMPPRocky XAMPPRocky closed this May 4, 2021
@khyperia khyperia removed their request for review May 31, 2021 09:25
@eddyb
Copy link
Contributor

eddyb commented Nov 10, 2023

(Re-opened temporarily to force push a rebase - the presence of the atomics branch, with a massive merge anomaly, was getting in the way of using jj on this repo, so I felt I had to untangle it)

@eddyb eddyb closed this Nov 10, 2023
@XAMPPRocky
Copy link
Member Author

@eddyb I can delete the branch if that helps.

@eddyb
Copy link
Contributor

eddyb commented Nov 10, 2023

I can delete the branch if that helps.

@XAMPPRocky Only if you think it's been obsoleted by e.g.:

(even if you delete the branch now, I still thing my cleanup of it was worth it, because the PR doesn't look extremely broken - idk how much that persists after deleting the branch, but better safe than sorry)

@XAMPPRocky XAMPPRocky deleted the atomics branch November 13, 2023 08:10
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