bcraig added a comment. > C++ atomics are explicitly designed to avoid problems with touching adjacent > bytes: if `atomic<T>` where `sizeof(T) == 1` requires a 4-byte `cmpxchg` then > it's up to the frontend to make sure `sizeof<atomic<T>> >= 4` (or something > equivalent such as making it non-lock-free).
I was aware of the C++11 wording on this, but for some reason, I was under the impression that the C11 wording didn't give the sizeof(T) latitude that C++11 did. A quick glance at the C spec tells me that I was under the wrong impression though. sizeof(_Atomic char) doesn't have to be the same as sizeof(char). So assuming the front end does the right per-target thing, _Atomic char (and std::atomic<char>) on MIPS and Hexagon probably _should_ be 4 bytes, and the IR should only do a 4 byte (or larger) cmpxchg. All the loads and stores would be "solicited" loads and stores. The back-end shouldn't need to do any masking, because the extra bytes are basically just padding. Long story short... continue as you were. Code looks fine. Ignore the crazy drive-by reviewer :) ================ Comment at: lib/Frontend/InitPreprocessor.cpp:305 @@ +304,3 @@ + if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 && + TypeWidth <= InlineWidth) + return Always; ---------------- jfb wrote: > bcraig wrote: > > jfb wrote: > > > bcraig wrote: > > > > jyknight wrote: > > > > > jfb wrote: > > > > > > bcraig wrote: > > > > > > > On some targets (like Hexagon), 4-byte values are cheap to > > > > > > > inline, but 1-byte values are not. Clang is spotty about > > > > > > > checking this, but TargetInfo::hasBuiltinAtomic seems like the > > > > > > > right function to ask, if you have access to it. > > > > > > You're commenting on: > > > > > > ``` > > > > > > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == > > > > > > 0 && > > > > > > TypeWidth <= InlineWidth) > > > > > > ``` > > > > > > ? > > > > > > > > > > > > Are you asking for a separate change, or for a change to my patch? > > > > > That issue in clang's purview in any case; if hexagon wants to lower > > > > > a 1-byte cmpxchg to a compiler runtime function, it should do so in > > > > > its backend. > > > > I am asking for a change to this patch. Don't assume that all widths > > > > smaller than some value are inline-able and lock free, because that may > > > > not be the case. A 4 byte cmpxchg could be lock free, while a 1 byte > > > > cmpxchg may not be lock free. > > > Are you asking me to change this line: > > > ``` > > > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 && > > > TypeWidth <= InlineWidth) > > > ``` > > > ? > > > > > > In what way? > > > > > > > > > Please be more specific. > > Yes, please change this line... > > > > if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 && > > TypeWidth <= InlineWidth) > > > > ... to something more like ... > > if(TI.hasBuiltinAtomic(TypeWidth, TypeAlign)) > > > > You will need to get the TargetInfo class into the method, and you should > > ensure that TypeWidth and TypeAlign are measured in terms of bits, because > > that is what TypeInfo::hasBuiltinAtomic is expecting. > > > > Using TargetInfo::hasBuiltinAtomic will let Targets have explicit control > > over what is and is not always lock free. The default implementation also > > happens to be pretty reasonable. > That's entirely unrelated to my change. My change is designed to *support* > the use case you allude to even though right now LLVM doesn't have targets > which exercise this usecase. Since you aren't changing the logic here, I'm find dropping this issue from this review. http://reviews.llvm.org/D17950 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
