Skip to content

Commit

Permalink
Fixes #357, #356: Implement, remove and update assignment operators a…
Browse files Browse the repository at this point in the history
…s necessary

* Made sure all (relevant) proxies have a (`noexcept`) move-assignment operator.
* Made sure all (relevant) proxies have no copy-assignment operator.
* Ensured move assignment operators regard all members, including `hold_pc_refcount_unit`.
* primary context assignment operators now actually return primary context references...
  • Loading branch information
eyalroz committed Jun 20, 2022
1 parent 78aae23 commit 58a8ef6
Show file tree
Hide file tree
Showing 10 changed files with 92 additions and 40 deletions.
6 changes: 5 additions & 1 deletion .github/action-scripts/install-cuda-windows.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,14 @@ if (!$?) {
# Store the CUDA_PATH in the environment for the current session, to be forwarded in the action.
$CUDA_PATH = "C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v$($CUDA_MAJOR).$($CUDA_MINOR)"
$CUDA_PATH_VX_Y = "CUDA_PATH_V$($CUDA_MAJOR)_$($CUDA_MINOR)"
$NVTOOLSEXT_PATH = "C:\Program Files\NVIDIA GPU Computing Toolkit\nvToolsExt"
# Set environmental variables in this session
$env:CUDA_PATH = "$($CUDA_PATH)"
$env:CUDA_PATH_VX_Y = "$($CUDA_PATH_VX_Y)"
$env:NVTOOLSEXT_PATH = "$($NVTOOLSEXT_PATH)"
Write-Output "CUDA_PATH $($CUDA_PATH)"
Write-Output "CUDA_PATH_VX_Y $($CUDA_PATH_VX_Y)"
Write-Output "NVTOOLSEXT_PATH $($NVTOOLSEXT_PATH)"

# PATH needs updating elsewhere, anything in here won't persist.
# Append $CUDA_PATH/bin to path.
Expand All @@ -183,8 +186,9 @@ Write-Output "CUDA_PATH_VX_Y $($CUDA_PATH_VX_Y)"
# If executing on github actions, emit the appropriate echo statements to update environment variables
if (Test-Path "env:GITHUB_ACTIONS") {
# Set paths for subsequent steps, using $env:CUDA_PATH
echo "Adding CUDA to CUDA_PATH, CUDA_PATH_X_Y and PATH"
echo "Adding CUDA-related environment entries: CUDA_PATH, NVTOOLSEXT_PATH"
echo "CUDA_PATH=$env:CUDA_PATH" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
echo "NVTOOLSEXT_PATH=$env:NVTOOLSEXT_PATH" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
echo "$env:CUDA_PATH_VX_Y=$env:CUDA_PATH" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
echo "$env:CUDA_PATH/bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
}
Expand Down
13 changes: 1 addition & 12 deletions src/cuda/api/context.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -648,18 +648,7 @@ class context_t {

public: // operators

context_t& operator=(const context_t& other)
{
if (owning_) {
context::detail_::destroy(handle_);
}
device_id_ = other.device_id_;
handle_ = other.handle_;
owning_ = false;
return *this;
}

// Deleted since the handle_t and handle_t are constant
context_t& operator=(const context_t&) = delete;
context_t& operator=(context_t&& other) noexcept
{
::std::swap(device_id_, other.device_id_);
Expand Down
22 changes: 15 additions & 7 deletions src/cuda/api/event.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -284,17 +284,25 @@ class event_t {

public: // operators

event_t& operator=(const event_t& other) = delete;
event_t& operator=(event_t&& other) = delete;
event_t& operator=(const event_t&) = delete;
event_t& operator=(event_t&& other) noexcept
{
::std::swap(device_id_, other.device_id_);
::std::swap(context_handle_, other.context_handle_);
::std::swap(handle_, other.handle_);
::std::swap(owning, other.owning);
::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit);
return *this;
}

protected: // data members
const device::id_t device_id_;
const context::handle_t context_handle_;
const event::handle_t handle_;
bool owning;
device::id_t device_id_;
context::handle_t context_handle_;
event::handle_t handle_;
bool owning;
// this field is mutable only for enabling move construction; other
// than in that case it must not be altered
bool holds_pc_refcount_unit;
bool holds_pc_refcount_unit;
// When context_handle_ is the handle of a primary context, this event may
// be "keeping that context alive" through the refcount - in which case
// it must release its refcount unit on destruction
Expand Down
13 changes: 13 additions & 0 deletions src/cuda/api/kernel.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,19 @@ class kernel_t {
kernel::handle_t handle() const noexcept { return handle_; }
#endif

public: // operators

kernel_t& operator=(const kernel_t&) = delete;
kernel_t& operator=(kernel_t&& other) noexcept
{
::std::swap(device_id_, other.device_id_);
::std::swap(context_handle_, other.context_handle_);
::std::swap(handle_, other.handle_);
::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit);
return *this;
}


public: // non-mutators

VIRTUAL_UNLESS_CAN_GET_APRIORI_KERNEL_HANDLE
Expand Down
19 changes: 13 additions & 6 deletions src/cuda/api/link.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,21 @@ class link_t {

public: // operators

link_t& operator=(const link_t& other) = delete;
link_t& operator=(link_t&& other) = delete;
link_t& operator=(const link_t&) = delete;
link_t& operator=(link_t&& other) noexcept
{
::std::swap(context_handle_, other.context_handle_);
::std::swap(handle_, other.handle_);
::std::swap(options_, other.options_);
::std::swap(owning, owning);
return *this;
}

protected: // data members
const context::handle_t context_handle_;
const link::handle_t handle_;
link::options_t options_;
bool owning;
context::handle_t context_handle_;
link::handle_t handle_;
link::options_t options_;
bool owning;
// this field is mutable only for enabling move construction; other
// than in that case it must not be altered
};
Expand Down
13 changes: 11 additions & 2 deletions src/cuda/api/module.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,17 @@ class module_t {

public: // operators

module_t& operator=(const module_t& other) = delete;
module_t& operator=(module_t&& other) = delete;
module_t& operator=(const module_t&) = delete;
module_t& operator=(module_t&& other) noexcept
{
::std::swap(device_id_, other.device_id_);
::std::swap(context_handle_, other.context_handle_);
::std::swap(handle_, other.handle_);
::std::swap(options_, other.options_);
::std::swap(owning_, other.owning_);
::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit);
return *this;
}

protected: // data members
device::id_t device_id_;
Expand Down
12 changes: 8 additions & 4 deletions src/cuda/api/multi_wrapper_impls/module.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ inline cuda::kernel_t module_t::get_kernel(const char* name) const

namespace module {

namespace detail_{
namespace detail_ {

template <typename Creator>
module_t create(const context_t& context, const void* module_data, Creator creator_function)
Expand All @@ -61,10 +61,14 @@ module_t create(const context_t& context, const void* module_data, Creator creat
+ cuda::detail_::ptr_as_hex(module_data)
+ " within " + context::detail_::identify(context));
bool do_take_ownership { true };
bool doesnt_hold_pc_refcount_unit { false };
// TODO: Do we want to allow holding a refcount unit here, if context is
// the primary context?

// TODO: Make sure the default-constructed options correspond to what cuModuleLoadData uses as defaults
return detail_::construct(
context.device_id(), context.handle(), new_module_handle,
link::options_t{}, do_take_ownership);
return detail_::wrap(
context.device_id(), context.handle(), new_module_handle,
link::options_t{}, do_take_ownership, doesnt_hold_pc_refcount_unit);
}


Expand Down
4 changes: 2 additions & 2 deletions src/cuda/api/primary_context.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ class primary_context_t : public context_t {

public: // operators

context_t& operator=(const primary_context_t& other) = delete;
context_t& operator=(primary_context_t&& other) = delete;
primary_context_t& operator=(const primary_context_t& other) = delete;
primary_context_t& operator=(primary_context_t&& other) = default;

public: // mutators of the proxied primary context, but not of the proxy

Expand Down
20 changes: 14 additions & 6 deletions src/cuda/api/stream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,15 @@ class stream_t {
// TODO: Do we really want to allow assignments? Hmm... probably not, it's
// too risky - someone might destroy one of the streams and use the others
stream_t& operator=(const stream_t& other) = delete;
stream_t& operator=(stream_t& other) = delete;
stream_t& operator=(stream_t& other) noexcept
{
::std::swap(device_id_, other.device_id_);
::std::swap(context_handle_, other.context_handle_);
::std::swap(handle_, other.handle_);
::std::swap(owning, other.owning);
::std::swap(holds_pc_refcount_unit, holds_pc_refcount_unit);
return *this;
}

public: // friendship

Expand All @@ -864,11 +872,11 @@ class stream_t {
}

protected: // data members
const device::id_t device_id_;
const context::handle_t context_handle_;
const stream::handle_t handle_;
bool owning;
bool holds_pc_refcount_unit;
device::id_t device_id_;
context::handle_t context_handle_;
stream::handle_t handle_;
bool owning;
bool holds_pc_refcount_unit;
// When context_handle_ is the handle of a primary context, this event may
// be "keeping that context alive" through the refcount - in which case
// it must release its refcount unit on destruction
Expand Down
10 changes: 10 additions & 0 deletions src/cuda/nvrtc/compilation_options.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,22 @@ struct compilation_options_t {
/**
* A sequence of directories to be searched for headers. These paths are searched _after_ the
* list of headers given to nvrtcCreateProgram.
*
* @note The members here are `std::string`'s rather than `const char*` or `std::string_view`'s,
* since this class is a value-type, and cannot rely someone else keeping these strings alive.
*
* @todo In C++17, consider making the elements `std::filesystem::path`'s.
*/
::std::vector<::std::string> additional_include_paths;

/**
* Header files to preinclude during preprocessing of the source.
*
* @note The members here are `std::string`'s rather than `const char*` or `std::string_view`'s,
* since this class is a value-type, and cannot rely someone else keeping these strings alive.
*
* @todo In C++17, consider making the elements `std::filesystem::path`'s.
*
* @todo Check how these strings are interpreted. Do they need quotation marks? brackets? full paths?
*/
::std::vector<::std::string> preinclude_files;
Expand Down

0 comments on commit 58a8ef6

Please sign in to comment.