[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

[John McCall via Phabricator via cfe-commits]

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