CHAI icon indicating copy to clipboard operation
CHAI copied to clipboard

Consider removing disabled execution spaces from enum

Open adayton1 opened this issue 2 years ago • 1 comments

We just hit a case where we were pulling out a raw pointer using chai::ExecutionSpace::GPU, and it was greater than NUM_EXECUTION_SPACES since CHAI was built with CUDA disabled. Totally a bug on our end, but I think it would be preferable for uses of execution spaces that are disabled to not even compile. This would involve the following code:

enum ExecutionSpace {
  /*! Default, no execution space. */
  NONE = 0,
  /*! Executing in CPU space */
  CPU,
#if defined(CHAI_ENABLE_CUDA) || defined(CHAI_ENABLE_HIP) || defined(CHAI_ENABLE_GPU_SIMULATION_MODE)
  /*! Execution in GPU space */
  GPU,
#endif
#if defined(CHAI_ENABLE_UM)
  UM,
#endif
#if defined(CHAI_ENABLE_PINNED)
  PINNED,
#endif
  // NUM_EXECUTION_SPACES should always be last!
  /*! Used to count total number of spaces */
  NUM_EXECUTION_SPACES
#if !defined(CHAI_ENABLE_CUDA) && !defined(CHAI_ENABLE_HIP) && !defined(CHAI_ENABLE_GPU_SIMULATION_MODE)
  ,GPU
#endif
#if !defined(CHAI_ENABLE_UM)
  ,UM
#endif
#if !defined(CHAI_ENABLE_PINNED)
  ,PINNED
#endif
};

The new code would remove entries after NUM_EXECUTION_SPACES:

enum ExecutionSpace {
  /*! Default, no execution space. */
  NONE = 0,
  /*! Executing in CPU space */
  CPU,
#if defined(CHAI_ENABLE_CUDA) || defined(CHAI_ENABLE_HIP) || defined(CHAI_ENABLE_GPU_SIMULATION_MODE)
  /*! Execution in GPU space */
  GPU,
#endif
#if defined(CHAI_ENABLE_UM)
  UM,
#endif
#if defined(CHAI_ENABLE_PINNED)
  PINNED,
#endif
  // NUM_EXECUTION_SPACES should always be last!
  /*! Used to count total number of spaces */
  NUM_EXECUTION_SPACES
};

adayton1 avatar Apr 18 '23 19:04 adayton1

I think actually having those symbols defined as extern instances of ExecutionSpace that defaulted to a reasonable alternative choice would be a good alternative. Having those be simple non-const externs would even allow for runtime-configuration of what they should be defaulted to.

robinson96 avatar Apr 18 '23 20:04 robinson96