tra added inline comments.

================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7362
+  if (FD->hasAttr<NVPTXKernelAttr>()) {
+    addNVVMMetadata(F, "kernel", 1);
+  }
----------------
jhuber6 wrote:
> tra wrote:
> > How does AMDGPU track kernels? It may be a good opportunity to stop using 
> > metadata for this if we can use a better suited mechanism. E.g. a function 
> > attribute or a calling convention.
> > 
> > 
> AMDGPU uses a calling convention, which is probably a better option. I don't 
> know how this still gets reduced in the back-end though.
OK. Switching from metadata to a new calling convention would be nice, but it 
is likely a bit more complicated and can be handled separately if/when we 
decide to do it. It's not needed for your purposes. 



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4872
+  if (AL.getKind() == ParsedAttr::AT_NVPTXKernel)
+    D->addAttr(::new (S.Context) NVPTXKernelAttr(S.Context, AL));
+  else
----------------
jhuber6 wrote:
> tra wrote:
> > I'm tempted to `addAttr(CUDAGlobal)` here, effectively making 
> > `nvptx_kernel` a target-specific alias for it, so we're guaranteed that 
> > they both are handled exactly the same everywhere. 
> > On the other hand, it all may be moot -- without CUDA compilation mode, 
> > `CUDAGlobal` handling will be different in this compilation mode.
> > 
> > Can CUDAGlobal itself be allowed to be used as a target-specific attribute 
> > for NVPTX in C++ mode?
> > 
> > I think, if possible, we should ideally have only one attribute doing the 
> > job, even if it may have somewhat different use cases in CUDA vs C++ 
> > compilation modes.
> > 
> > 
> Yeah that's what I was thinking. Right now we only parse and check all the 
> CUDA attributes in the CUDA language mode. I could change it to allow them 
> whenever we're compiling for the `NVPTX` architecture instead. I don't think 
> for the vast majority it would have any significant effect.
Let's give it a try.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140226

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to