[PATCH] D99301: [HIP] add __builtin_get_device_side_mangled_name

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: clang/include/clang/Basic/Builtins.h:39
   OMP_LANG = 0x80,    // builtin requires OpenMP.
+  HIP_LANG = 0x100,   // builtin requires HIP.
   ALL_LANGUAGES = C_LANG | CXX_LANG | OBJC_LANG, // builtin for all languages.
----------------
tra wrote:
> The issue is common for both CUDA and HIP. 
will rename it to CUDA_LANG


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8310
+def err_hip_invalid_args_builtin_mangled_name : Error<
+    "invalid argument: expect a device-side function or global variable">;
+
----------------
tra wrote:
> Nit. I'd rephrase it in terms of what makes the argument invalid. E.g. `symbol must be a device-side function or global variable`.
will do


================
Comment at: clang/lib/Basic/Builtins.cpp:78
   bool OpenMPUnsupported = !LangOpts.OpenMP && BuiltinInfo.Langs == OMP_LANG;
+  bool HIPUnsupported = !LangOpts.HIP && BuiltinInfo.Langs == HIP_LANG;
   bool CPlusPlusUnsupported =
----------------
tra wrote:
> tra wrote:
> > Wow! The density of negations in this function is impressive.
> > 
> > 
> Please enable it for CUDA, too.
will do


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1980-1983
+      if (!D->hasAttr<CUDAGlobalAttr>() && !D->hasAttr<CUDADeviceAttr>() &&
+          !D->hasAttr<CUDAConstantAttr>() && !D->hasAttr<HIPManagedAttr>())
+        return false;
+      return true;
----------------
tra wrote:
> ```
>   return D->hasAttr<CUDAGlobalAttr>() || D->hasAttr<CUDADeviceAttr>() ||
>           D->hasAttr<CUDAConstantAttr>() || D->hasAttr<HIPManagedAttr>();
> ```
will do


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99301/new/

https://reviews.llvm.org/D99301