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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
