- User Since
- Jan 27 2014, 9:36 AM (346 w, 6 d)
Sat, Sep 19
Fri, Sep 18
Thu, Sep 17
Please provide documentation in this patch.
Tue, Sep 8
Generally this looks good, but I didn't go into details.
It would be good to have an MS person like @STL_MSFT check that the std tests match, at least for what they've implemented.
On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch.
Mon, Sep 7
Thu, Sep 3
Tue, Sep 1
Is this really NFC?
Sat, Aug 29
MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270.
Fri, Aug 28
Please consider these changes, and whether this is relevant as implemented: http://wg21.link/p0270
Thu, Aug 27
I'll wait 5 days to do this, since our process says:
Remove the "suppress this" in release notes.
Wed, Aug 26
Re-upload with full context.
As Vedant suggested, make it part of 'integer' sanitizer.
Address Richard's comments.
This is probably still useful, if someone wants to pick it up.
I don't think the world is ready for this.
Mon, Aug 24
Ping, I think I've addressed all comments here.
Aug 18 2020
Aug 17 2020
Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?
Aug 14 2020
@ldionne should sign off on the libcxx test, but the rest seems fine to me. Eventually we probably want to enable something, but need to be consistent about it.
The typo fix is fine, I'm not sure it's worth fixing variable names (since there's an ongoing discussion about renaming everything anyways).
FWIW C++ doesn't really have a lattice anymore: https://wg21.link/p0418
Aug 13 2020
Actually I think any subsequent updates to tests can be done in a follow-up patch, since I'm not changing the status-quo on address space here.
Fix a test.
I'd expect us to go the other way, and allow this. There have been patches in the past to allow usage of atomics with float. Could you look with their authors and see what they advise here?
Update overloading as discussed: on the original builtins, and separate the _sized variant, making its 4th parameter non-optional. If this looks good, I'll need to update codege for a few builtins (to handle volatile), as well as add tests for their codegen and address space (which should already work, but isn't tested).
Can you please add tests for this?
Aug 12 2020
Does this x86 change actually match what the runtime does in the same file?
Aug 11 2020
Aug 10 2020
You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.
I didn't check that all of these compare all the required state for all opcodes.
Aug 5 2020
Remove restrict, update docs, call isCompleteType
Use 'access size' instead of 'element size'.
Add loop test requested by Vedant
Do UBSan change suggested by Vedant.
Aug 4 2020
Thanks for the detailed comments, I think I've addressed all of them! I also added UBSan support to check the builtin invocation. I think this patch is pretty much ready to go. A follow-up will need to add the support functions to compiler-rt (they're currently optional, as per https://reviews.llvm.org/D33240), and in cases where size is known we can inline them (as we do for memcpy and friends).
Address Richard's comments, add UBSan support.
Aug 3 2020
Jul 31 2020
This is almost ready I think!
There are a few things still open, I'd love feedback on them.
Address Richard's comments.
Is that true of all vector bfloat implementations? It seems like arithmetic on these types is something implementations would likely support.
Jul 29 2020
Jul 28 2020
Jul 27 2020
I've addressed @rsmith @rjmccall suggestions (unless noted), thanks!
An open question: as of 6e4aa1e48138182685431c76184dfc36e620aea2 @dneilson added an assertion on CreateElementUnorderedAtomicMemCpy to check that the pointer arguments have alignments of at least the element size. That makes sense when the IR is only ever built internally to LLVM, but now that I'm adding a builtin it's more of a dynamic property. Should I also force this in the frontend (understanding that alignment isn't always well known at compile time), or should simply assume that the alignment is correct because it's a dynamic property?
Jul 23 2020
Do you think it'd be useful to have different guarantees for different operands? I guess it could come up, but it'd be a whole lot of extra complexity that I can't imagine we'd ever support.
You mean, if element_size is passed then you get different guarantees?
No, sorry, I mean different guarantees for the different pointer operands. In principle, we could allow you to say that the memcpy has to be done with 4-byte accesses from the source and 2-byte accesses to the destination. That's implementable but a lot of work.
My point is that this has nothing to do with the ordinary semantics of _Atomic. You're basically just looking at the word "atomic" and saying that, hey, a minimum access size is sortof related to atomicity.
If you want this to be able to control the minimum access size, you should allow that to be passed in as an optional argument instead.
Jul 22 2020
Address all but one of John's comments
Follow John's suggestions