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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
