[PATCH] D79754: [OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 1

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:3221
+        !hasAttr<CUDAHostAttr>()) ||
+       Context.getTargetInfo().getTriple().isAMDGCN()) &&
       !(BuiltinID == Builtin::BIprintf || BuiltinID == Builtin::BImalloc))
----------------
sameerds wrote:
> This seems awkward to me. Why mix it up with only CUDA and HIP? The earlier factoring is better, where CUDA/HIP took care of their own business, and the catch-all case of AMDGCN was a separate clause by itself. It doesn't matter that the builtins being checked for AMDGCN on OpenMP are //currently// identical to CUDA/HIP. When this situation later changes (I am sure OpenMP will support more builtins), we will have to split it out again anyway.
This clause seems to assume that AMDGCN automatically means OpenMP whenever it is not HIP. That does not sound right. Is there a Context.getLangOpts().OpenMP flag? If not, why not? There also seems to be an Opts.OpenMPIsDevice ... perhaps that could be used here to write a separate OpenMP clause?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79754