tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

LGTM. Next step is to figure out what various 
__nv_tex_surf_handler(<string>...) maps to for various strings (there are ~110 
of them in CUDA-10.2) and implement its replacement. I think we should be able 
to do it in the wrapper file.



================
Comment at: clang/lib/Headers/__clang_cuda_runtime_wrapper.h:82-94
 #undef __CUDACC__
 #if CUDA_VERSION < 9000
 #define __CUDABE__
 #else
+#define __CUDACC__
 #define __CUDA_LIBDEVICE__
 #endif
----------------
hliao wrote:
> tra wrote:
> > hliao wrote:
> > > hliao wrote:
> > > > tra wrote:
> > > > > hliao wrote:
> > > > > > tra wrote:
> > > > > > > Please add comments on why __CUDACC__ is needed for 
> > > > > > > driver_types.h here? AFAICT, driver_types.h does not have any 
> > > > > > > conditionals that depend on __CUDACC__. What happens if it's not 
> > > > > > > defined.
> > > > > > > 
> > > > > > > 
> > > > > > `driver_types.h` includes `host_defines.h`, where macros 
> > > > > > `__device_builtin_surface_type__` and 
> > > > > > `__device_builtin_texture_type__` are conditional defined if 
> > > > > > `__CUDACC__`.
> > > > > > 
> > > > > > The following is extracted from `cuda/crt/host_defines.h`
> > > > > > 
> > > > > > ```
> > > > > > #if !defined(__CUDACC__)
> > > > > > #define __device_builtin__
> > > > > > #define __device_builtin_texture_type__
> > > > > > #define __device_builtin_surface_type__
> > > > > > #define __cudart_builtin__
> > > > > > #else /* defined(__CUDACC__) */
> > > > > > #define __device_builtin__ \
> > > > > >         __location__(device_builtin)
> > > > > > #define __device_builtin_texture_type__ \
> > > > > >         __location__(device_builtin_texture_type)
> > > > > > #define __device_builtin_surface_type__ \
> > > > > >         __location__(device_builtin_surface_type)
> > > > > > #define __cudart_builtin__ \
> > > > > >         __location__(cudart_builtin)
> > > > > > #endif /* !defined(__CUDACC__) */
> > > > > > ```
> > > > > My concern is -- what else is going to get defined? There are ~60 
> > > > > references to __CUDACC__ in CUDA-10.1 headers. The wrappers are 
> > > > > fragile enough that there's a good chance something may break. It 
> > > > > does not help that my CUDA build bot decided to die just after we 
> > > > > switched to work-from-home, so there will be no early warning if 
> > > > > something goes wrong.
> > > > > 
> > > > > If all we need are the macros above, we may just define them. 
> > > > Let me check all CUDA SDK through their dockers. Redefining sounds good 
> > > > me as wll.
> > > I checked headers from 7.0 to 10.0, `__device_builtin_texture_type__` and 
> > > `__builtin_builtin_surface_type__` are only defined with that attributes 
> > > if `__CUDACC__` is defined. As we only pre-define `__CUDA_ARCH__` in 
> > > clang but flip `__CUDACC__` on and off in the wrapper headers to 
> > > selectively reuse CUDA's headers. I would hear your suggestion on that.
> > > BTW, macros like `__device__` are defined regardless of `__CUDACC__` from 
> > > 7.0 to 10.0 as `__location(device)`. `__location__` is defined if 
> > > `__CUDACC__` is present. But, different from `__device__`, 
> > > `__device_builtin_texture_type__` is defined only `__CUDACC__` is defined.
> > `__device_builtin_texture_type__` is defined in `host_defines.h`, which 
> > does not seem to include any other files or does anything suspicious with 
> > `__CUDACC__`
> > 
> > It may be OK to move inclusion of `host_defines.h` to the point before 
> > `driver_types.h`, which happens to include the host_defines.h first, and 
> > define __CUDACC__ only around `host_defines.h`.
> > 
> > An alternative is to add the macros just after inclusion of `host_defines.h`
> > 
> > In either case please verify that these attributes are the only things 
> > that's changed by diffing output of `clang++ -x cuda /dev/null 
> > --cuda-host-only -dD -E -o -` before and after the change.
> With `__CUDACC__`, the only difference is the additional attributes added, 
> such as `device_builtin_texture_type`. Attributes like `cudart_builtin` are 
> also defined correctly. That should be used to start the support CUDART 
> features.
> I revised the change to include `host_defines.h` first and found there's no 
> changes from the one using `driver_types.h`. We should be OK for that change.
SGTM. Thank you for verifying this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76365



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

Reply via email to