[PATCH] D17950: Implement is_always_lock_free

[Ben Craig via cfe-commits]

bcraig added inline comments.

================
Comment at: lib/Frontend/InitPreprocessor.cpp:305
@@ +304,3 @@
+    if (TypeWidth == TypeAlign && (TypeWidth & (TypeWidth - 1)) == 0 &&
+        TypeWidth <= InlineWidth)
+      return Always;
----------------
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.


http://reviews.llvm.org/D17950