[PATCH] D17950: Implement is_always_lock_free

[Ben Craig via cfe-commits]

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