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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam marked 5 inline comments as done.
saiislam added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:3224
 
+  if (Context.getTargetInfo().getTriple().isAMDGCN() &&
+      Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID) &&
----------------
jdoerfert wrote:
> sameerds wrote:
> > arsenm wrote:
> > > This is also identical to the cuda handling above, can you merge them
> > It's not identical, because CUDA is filtering out host code and its check is target independent.
> > 
> > But then, Saiyed, it seems that the new patch disallows library builtins on all languages that reach this point, both on host and device code. Is that the intention? Does this point in the flow preclude any side-effects outside of "OpenMP on AMDGCN"?
> Yes, wasn't there an idea to have isGPU()?
@sameerds  this function returns a value indicating whether input function corresponds to a builtin function (returns BuiltinID), or not (returns 0) i.e. all conditions returning 0 are the exceptions where a valid BuiltinID can't be returned. The new condition only applies to non-OpenCL (line 3213) builtin functions which are not printf and malloc, and have been targeted for amdgcn.


================
Comment at: clang/lib/AST/Decl.cpp:3224
 
+  if (Context.getTargetInfo().getTriple().isAMDGCN() &&
+      Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID) &&
----------------
saiislam wrote:
> jdoerfert wrote:
> > sameerds wrote:
> > > arsenm wrote:
> > > > This is also identical to the cuda handling above, can you merge them
> > > It's not identical, because CUDA is filtering out host code and its check is target independent.
> > > 
> > > But then, Saiyed, it seems that the new patch disallows library builtins on all languages that reach this point, both on host and device code. Is that the intention? Does this point in the flow preclude any side-effects outside of "OpenMP on AMDGCN"?
> > Yes, wasn't there an idea to have isGPU()?
> @sameerds  this function returns a value indicating whether input function corresponds to a builtin function (returns BuiltinID), or not (returns 0) i.e. all conditions returning 0 are the exceptions where a valid BuiltinID can't be returned. The new condition only applies to non-OpenCL (line 3213) builtin functions which are not printf and malloc, and have been targeted for amdgcn.
@jdoerfert  Can you please elaborate on isGPU() idea? I am not aware about it.
Guessing by the name, I have added isGPU() as a wrapper over isNVPTX() and isAMDGCN() in the next revision.


================
Comment at: clang/lib/AST/Decl.cpp:3225
+  if (Context.getTargetInfo().getTriple().isAMDGCN() &&
+      Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID) &&
+      !(BuiltinID == Builtin::BIprintf || BuiltinID == Builtin::BImalloc))
----------------
this condition is not required because it has already been checked in line #3200 earlier.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3162
+      Opts.OpenMPIsDevice && (T.isNVPTX() || T.isAMDGCN()) &&
       Args.hasArg(options::OPT_fopenmp_cuda_force_full_runtime);
 
----------------
sameerds wrote:
> jdoerfert wrote:
> > Can we please not call it CUDA mode for non-CUDA targets? Or do you expect the compilation to "identify" as CUDA compilation?
> I suspect it's just a lot of water under the bridge. All over Clang, HIP has traditionally co-opted CUDA code without introducing new identifiers, like "-fcuda-is-device". It won't be easy to redo that now, and definitely looks out of scope for this change. A typical HIP compilation usually does identify itself as a HIP compilation like setting the isHIP flag. This allows the frontend to distinguish between CUDA and HIP when it matters.
@jdoerfert [[ https://clang.llvm.org/docs/OpenMPSupport.html#data-sharing-modes | OpenMP support document of clang ]] defines two data sharing modes for cuda devices: CUDA and Generic mode. NVPTX and AMDGCN both are cuda devices. Doesn't it make sense to not refactor CUDA mode as of now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79754