rjmccall added inline comments.
================
Comment at: include/clang/Basic/SyncScope.h:1
+//===--- SyncScope.h - atomic synchronization scopes ------------*- C++
-*-===//
+//
----------------
Capitalization.
================
Comment at: include/clang/Basic/SyncScope.h:20
+
+namespace SyncScope {
+
----------------
LLVM uses this namespace pattern in code that predates the addition of scoped
enums to C++. Those days are behind us; we should just use a scoped enum.
================
Comment at: include/clang/Basic/SyncScope.h:22
+
+/// \brief Defines the synch scope values used by the atomic instructions.
+///
----------------
It defines the synch scope values used by the atomic builtins and expressions.
LLVM's headers define the values used by the instructions.
================
Comment at: include/clang/Basic/SyncScope.h:25-29
+ SingleThread = 0,
+ WorkGroup = 1,
+ Device = 2,
+ System = 3,
+ SubGroup = 4,
----------------
Anastasia wrote:
> t-tye wrote:
> > Since the builtins are being named as __opencl then should these also be
> > named as opencl_ since they are the memory scopes for OpenCL using the
> > OpenCL numeric values?
> >
> > If another language wants to use memory scopes, would it then add its own
> > langx_* names in a similar way that is done for address spaces where the
> > LangAS enumeration type has values for each distinct language. Each target
> > is then responsible for mapping each language scope to the appropriate
> > target specific scope as is done for address spaces?
> >
> > If so then the builtins are really supporting the concept of memory scopes
> > and are not language specific as this enumeration can support multiple
> > languages in the same way as the LangAS enumeration supports multiple
> > languages. If so the builtins would be better named to reflect this as
> > @b-sumner suggested.
> We generally prefix the names of OpenCL specific implementations. So perhaps
> we should do some renaming here in case we don't intend this to be generic
> implementation.
I agree that we should name the OpenCL-specific ones, like WorkGroup, with an
OpenCL prefix.
================
Comment at: include/clang/Basic/SyncScope.h:32
+
+inline unsigned getMaxValue(void) {
+ return SubGroup;
----------------
This is C++; please just use () instead of (void).
================
Comment at: lib/CodeGen/CGAtomic.cpp:503
+ ->getAs<BuiltinType>()
+ ->isSignedIntegerType();
+}
----------------
None of the .getTypePtr() stuff here is necessary.
This function shouldn't really be necessary. I would encourage you to add a
getValueType() accessor to AtomicExpr:
QualType AtomicExpr::getValueType() const {
auto T = getPtr()->getType()->castTo<PointerType>()->getPointeeType();
if (auto AT = T->getAs<AtomicType>()) {
return AT->getValueType();
} else {
return T;
}
}
You can then just use E->getValueType()->isSignedIntegerType() and eliminate
this helper function.
================
Comment at: lib/CodeGen/CGAtomic.cpp:919
+ ->getPointeeType()
+ .getAddressSpace();
+ auto *DestType = T->getPointerElementType()->getPointerTo(DestAS);
----------------
Again you're using getTypePtr() unnecessarily.
The check should be whether the AST-level address spaces match, not whether the
lowered address spaces match.
Please pass E->getType() instead of E here.
You should remove the DoIt parameter and just check E->isOpenCL() (where E is
the AtomicExpr already in scope).
================
Comment at: lib/CodeGen/CGCall.cpp:3911
+ V = Builder.CreateBitOrPointerCast(V,
+ IRFuncTy->getParamType(FirstIRArg));
----------------
No. Callers should ensure that they've added the right argument type, at least
at the level of address spaces.
================
Comment at: lib/CodeGen/TargetInfo.h:266
+ /// Get the syncscope name used in LLVM IR.
+ virtual llvm::StringRef getSyncScopeName(SyncScope::ID S) const;
};
----------------
Why does this return a StringRef instead of an llvm::SynchronizationScope?
https://reviews.llvm.org/D28691
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
