Re: [Starpu-devel] Patch request for the multiformat interface

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Nathalie Furmento, le Wed 29 Feb 2012 11:42:48 +0100, a écrit :
> Index: include/starpu_data_interfaces.h
> ===================================================================
> --- include/starpu_data_interfaces.h  (r??vision 5892)
> +++ include/starpu_data_interfaces.h  (copie de travail)
> @@ -350,27 +350,19 @@
>  struct starpu_multiformat_data_interface_ops
>  {
>       size_t cpu_elemsize;
> -#ifdef STARPU_USE_OPENCL
>       size_t opencl_elemsize;
>       struct starpu_codelet *cpu_to_opencl_cl;
>       struct starpu_codelet *opencl_to_cpu_cl;
> -#endif
> -#ifdef STARPU_USE_CUDA
>       size_t cuda_elemsize;
>       struct starpu_codelet *cpu_to_cuda_cl;
>       struct starpu_codelet *cuda_to_cpu_cl;
> -#endif
>  };
>  
>  struct starpu_multiformat_interface
>  {
>       void *cpu_ptr;
> -#ifdef STARPU_USE_CUDA
>       void *cuda_ptr;
> -#endif
> -#ifdef STARPU_USE_OPENCL
>       void *opencl_ptr;
> -#endif
>       uint32_t nx;
>       struct starpu_multiformat_data_interface_ops *ops;
>  };
> @@ -378,15 +370,8 @@
>  void starpu_multiformat_data_register(starpu_data_handle_t *handle, 
> uint32_t home_node, void *ptr, uint32_t nobjects, struct 
> starpu_multiformat_data_interface_ops *format_ops);
>  
>  #define STARPU_MULTIFORMAT_GET_CPU_PTR(interface)  (((struct 
> starpu_multiformat_interface *)(interface))->cpu_ptr)
> -
> -#ifdef STARPU_USE_CUDA
>  #define STARPU_MULTIFORMAT_GET_CUDA_PTR(interface) (((struct 
> starpu_multiformat_interface *)(interface))->cuda_ptr)
> -#endif
> -
> -#ifdef STARPU_USE_OPENCL
>  #define STARPU_MULTIFORMAT_GET_OPENCL_PTR(interface) (((struct 
> starpu_multiformat_interface *)(interface))->opencl_ptr)
> -#endif
> -
>  #define STARPU_MULTIFORMAT_GET_NX(interface)  (((struct 
> starpu_multiformat_interface *)(interface))->nx)
>  
>  enum starpu_data_interface_id 
> starpu_handle_get_interface_id(starpu_data_handle_t handle);

That part, for sure, is fine, the interface shouldn't depend on whether
CUDA/OpenCL is enabled.

The rest, not necessarily:

> Index: src/core/sched_policy.c
> ===================================================================
> --- src/core/sched_policy.c   (r??vision 5892)
> +++ src/core/sched_policy.c   (copie de travail)
> @@ -357,31 +357,23 @@
>               {
>               case STARPU_CPU_RAM:
>                       STARPU_ASSERT(0);
> -#ifdef STARPU_USE_CUDA
>               case STARPU_CUDA_RAM:
>                       conversion_task->cl = mf_ops->cuda_to_cpu_cl;
>                       break;
> -#endif
> -#ifdef STARPU_USE_OPENCL
>               case STARPU_OPENCL_RAM:
>                       conversion_task->cl = mf_ops->opencl_to_cpu_cl;
>                       break;
> -#endif
>               default:
>                       fprintf(stderr, "Oops : %u\n", handle->mf_node);
>                       STARPU_ASSERT(0);
>               }
>               break;
> -#ifdef STARPU_USE_CUDA
>       case STARPU_CUDA_RAM:
>               conversion_task->cl = mf_ops->cpu_to_cuda_cl;
>               break;
> -#endif
> -#ifdef STARPU_USE_OPENCL
>       case STARPU_OPENCL_RAM:
>               conversion_task->cl = mf_ops->cpu_to_opencl_cl;
>               break;
> -#endif
>       case STARPU_SPU_LS: /* Not supported */
>       default:
>               STARPU_ASSERT(0);

For instance there it's better to error if we don't have CUDA/OpenCL but
somehow the manage to find such a node, and thus keep the ifdefs.

> Index: src/datawizard/interfaces/multiformat_interface.c
> ===================================================================
> --- src/datawizard/interfaces/multiformat_interface.c (r??vision 5892)
> +++ src/datawizard/interfaces/multiformat_interface.c (copie de travail)
> @@ -174,24 +174,16 @@
>                                     uint32_t nobjects,
>                                     struct 
> starpu_multiformat_data_interface_ops *format_ops)
>  {
> -#ifdef STARPU_USE_OPENCL
>       _starpu_codelet_check_deprecated_fields(format_ops->cpu_to_opencl_cl);
>       _starpu_codelet_check_deprecated_fields(format_ops->opencl_to_cpu_cl);
> -#endif
> -#ifdef STARPU_USE_CUDA
>       _starpu_codelet_check_deprecated_fields(format_ops->cpu_to_cuda_cl);
>       _starpu_codelet_check_deprecated_fields(format_ops->cuda_to_cpu_cl);
> -#endif
>  

These should indeed probably be always enabled, to always do the correction.

>       struct starpu_multiformat_interface multiformat =
>       {
>               .cpu_ptr    = ptr,
> -#ifdef STARPU_USE_CUDA
>               .cuda_ptr   = NULL,
> -#endif
> -#ifdef STARPu_USE_OPENCL
>               .opencl_ptr = NULL,
> -#endif
>               .nx         = nobjects,
>               .ops        = format_ops
>       };

That's fine too, it'd be set to NULL anyway.

> @@ -211,12 +203,8 @@
>  
>       return ((multiformat_a->nx == multiformat_b->nx)
>                       && (multiformat_a->ops->cpu_elemsize == 
> multiformat_b->ops->cpu_elemsize)
> -#ifdef STARPU_USE_CUDA
>                       && (multiformat_a->ops->cuda_elemsize == 
> multiformat_b->ops->cuda_elemsize)
> -#endif
> -#ifdef STARPU_USE_OPENCL
>                       && (multiformat_a->ops->opencl_elemsize == 
> multiformat_b->ops->opencl_elemsize)
> -#endif
>               );
>  }

These should probably not always be enabled, because their content might
be undefined, thus keep the ifdefs.

> Index: src/datawizard/interfaces/data_interface.c
> ===================================================================
> --- src/datawizard/interfaces/data_interface.c        (r??vision 5895)
> +++ src/datawizard/interfaces/data_interface.c        (copie de travail)
> @@ -482,16 +482,12 @@
>                       mf_ops = (struct 
> starpu_multiformat_data_interface_ops *) 
> handle->ops->get_mf_ops(format_interface);
>                       switch (node_kind)
>                       {
> -#ifdef STARPU_USE_CUDA
>                               case STARPU_CUDA_RAM:
>                                       cl = mf_ops->cuda_to_cpu_cl;
>                                       break;
> -#endif
> -#ifdef STARPU_USE_OPENCL
>                               case STARPU_OPENCL_RAM:
>                                       cl = mf_ops->opencl_to_cpu_cl;
>                                       break;
> -#endif
>                               case STARPU_CPU_RAM:      /* Impossible ! */
>                               case STARPU_SPU_LS:       /* Not supported */
>                               default:

It's better to error out, so keep the ifdefs.

Samuel