[PATCH] D17950: Implement is_always_lock_free

[JF Bastien via cfe-commits]

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