jfb added a comment. In http://reviews.llvm.org/D17950#377386, @cfe-commits wrote:
> I know that MIPS does that, and an out-of-tree implementation of hexagon > implements 1-byte cmpxchg in terms of the 4-byte version. The emulation > code isn't particularly small, and it seems reasonable to make it a > libcall. The emulation code seems sketchy from a correctness > perspective, as you end up generating unsolicited loads and stores on > adjacent bytes. Take a look at the thread on cfe-dev and llvm-dev named > "the as-if rule / perf vs. security" for some of the ramifications and > concerns surrounding unsolicited loads and stores. 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). This doesn't fall under "as-if". Whether clang does this correctly for the targets you have in mind is another issue. Whether LLVM's atomic instructions should assume C++ semantics in one way or another, i.e. whether they should assume the frontend generated code where padding can be overriden, is also a separate issue. In all of these cases I suggest filing a separate issue. This patch is the wrong place to address the issues you raise. They're not invalid issues, they're simply not related to http://reviews.llvm.org/D17950. ================ Comment at: lib/Frontend/InitPreprocessor.cpp:305 @@ +304,3 @@ + if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 && + TypeWidth <= InlineWidth) + return Always; ---------------- 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. http://reviews.llvm.org/D17950 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
