-
Notifications
You must be signed in to change notification settings - Fork 8
Phambinh/circular vmm pool #774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
1dc7141
41a8306
e5ad606
19dbdd3
76d7636
af977d0
f3ae560
7dbb362
8fe4743
33a92a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -116,6 +116,9 @@ limitations under the License. | |||||||||||||||||||||||||||||
| #include "xla/stream_executor/stream_executor.h" | ||||||||||||||||||||||||||||||
| #include "xla/stream_executor/sycl/sycl_platform_id.h" | ||||||||||||||||||||||||||||||
| #include "xla/stream_executor/vmm_device_address_allocator.h" | ||||||||||||||||||||||||||||||
| #if TENSORFLOW_USE_ROCM | ||||||||||||||||||||||||||||||
| #include "xla/stream_executor/rocm/circular_vmm_pool.h" | ||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||
| #include "xla/tsl/platform/env.h" | ||||||||||||||||||||||||||||||
| #include "xla/tsl/platform/env_time.h" | ||||||||||||||||||||||||||||||
| #include "xla/tsl/platform/logging.h" | ||||||||||||||||||||||||||||||
|
|
@@ -1515,6 +1518,160 @@ absl::Status GpuExecutable::ExecuteThunksWithVaRemapping( | |||||||||||||||||||||||||||||
| return absl::OkStatus(); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| absl::Status GpuExecutable::ExecuteThunksWithCircularVmmPool( | ||||||||||||||||||||||||||||||
| const BufferAllocations& buffer_allocations, | ||||||||||||||||||||||||||||||
| const ServiceExecutableRunOptions* run_options, | ||||||||||||||||||||||||||||||
| se::StreamExecutor* executor, int64_t unique_id, | ||||||||||||||||||||||||||||||
| Thunk::ExecutableSource executable_source, bool block_host_until_done) { | ||||||||||||||||||||||||||||||
| #if TENSORFLOW_USE_ROCM | ||||||||||||||||||||||||||||||
| CircularPoolState* pool_state = nullptr; | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| absl::MutexLock lock(&circular_pool_mutex_); | ||||||||||||||||||||||||||||||
| pool_state = &circular_pools_[executor]; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+1529
to
+1531
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: thread safety — The mutex If two threads call Consider either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved -- addressed in this revision. |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Cache buffer classification on first call (doesn't change between iters). | ||||||||||||||||||||||||||||||
| if (!pool_state->indexes_cached) { | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: data race in double-checked locking on
Either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved -- addressed in this revision. indexes_cached replaced by std::atomic initialized with proper acquire/release memory ordering for safe double-checked locking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved: |
||||||||||||||||||||||||||||||
| absl::MutexLock lock(&pool_state->mu); | ||||||||||||||||||||||||||||||
| if (!pool_state->indexes_cached) { | ||||||||||||||||||||||||||||||
| if (buffer_assignment_) { | ||||||||||||||||||||||||||||||
| for (BufferAllocation::Index idx : command_buffer_allocation_indexes_) { | ||||||||||||||||||||||||||||||
| const auto& alloc = buffer_assignment_->GetAllocation(idx); | ||||||||||||||||||||||||||||||
| if (alloc.is_constant() || alloc.size() == 0) continue; | ||||||||||||||||||||||||||||||
| pool_state->pool_indexes.insert(idx); | ||||||||||||||||||||||||||||||
| if (alloc.is_entry_computation_parameter() || | ||||||||||||||||||||||||||||||
| alloc.maybe_live_out()) { | ||||||||||||||||||||||||||||||
| pool_state->copy_indexes.insert(idx); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| pool_state->indexes_cached = true; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| const auto& pool_indexes = pool_state->pool_indexes; | ||||||||||||||||||||||||||||||
| const auto& copy_indexes = pool_state->copy_indexes; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Initialize pool on first use. | ||||||||||||||||||||||||||||||
| if (pool_state->pool == nullptr) { | ||||||||||||||||||||||||||||||
| int num_slots = has_module() | ||||||||||||||||||||||||||||||
| ? module_config().debug_options().xla_gpu_circular_vmm_pool_slots() | ||||||||||||||||||||||||||||||
| : 1; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (pool_indexes.empty()) { | ||||||||||||||||||||||||||||||
| return ExecuteThunksImpl( | ||||||||||||||||||||||||||||||
| has_module() ? &module_config().debug_options() : nullptr, | ||||||||||||||||||||||||||||||
| module_name_, unique_id, *thunk_executor_, executable_source, | ||||||||||||||||||||||||||||||
| run_options, buffer_allocations, block_host_until_done, | ||||||||||||||||||||||||||||||
| execution_stream_ids_, collective_memory_cache_); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| std::vector<uint64_t> buffer_sizes; | ||||||||||||||||||||||||||||||
| buffer_sizes.reserve(pool_indexes.size()); | ||||||||||||||||||||||||||||||
| for (BufferAllocation::Index idx : pool_indexes) { | ||||||||||||||||||||||||||||||
| buffer_sizes.push_back(buffer_allocations.GetDeviceAddress(idx).size()); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| TF_ASSIGN_OR_RETURN( | ||||||||||||||||||||||||||||||
| auto pool, | ||||||||||||||||||||||||||||||
| se::gpu::CircularVmmPool::Create(executor, buffer_sizes, num_slots)); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| LOG(INFO) << absl::StrFormat( | ||||||||||||||||||||||||||||||
| "CircularVmmPool: created %d slots for module %s on device %d " | ||||||||||||||||||||||||||||||
| "(%d command buffer allocations)", | ||||||||||||||||||||||||||||||
| num_slots, module_name_, executor->device_ordinal(), | ||||||||||||||||||||||||||||||
| command_buffer_allocation_indexes_.size()); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| pool_state->pool = std::move(pool); | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: pool initialization race -- If two threads call This should use the same double-checked locking pattern as if (pool_state->pool == nullptr) {
absl::MutexLock lock(&pool_state->mu);
if (pool_state->pool == nullptr) {
// ... create pool ...
}
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved -- addressed in this revision. Pool initialization is now guarded by atomic initialized + per-pool mutex double-checked locking. The pool creation, index computation, and initialized flag are all protected within the same critical section. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved: pool initialization is now inside an atomic double-checked locking pattern with a per-pool mutex, eliminating the race. |
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| auto* pool = static_cast<se::gpu::CircularVmmPool*>(pool_state->pool.get()); | ||||||||||||||||||||||||||||||
| uint64_t iteration = pool_state->iteration_count.fetch_add(1); | ||||||||||||||||||||||||||||||
| int slot_idx = iteration % pool->num_slots(); | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Nit] |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Acquire next slot — non-blocking check of GPU timeline counter. | ||||||||||||||||||||||||||||||
| TF_ASSIGN_OR_RETURN(auto slot_addresses, pool->AcquireNextSlot(iteration)); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| VLOG(3) << absl::StrFormat( | ||||||||||||||||||||||||||||||
| "CircularVmmPool iter=%d slot=%d/%d: %d pool addrs", | ||||||||||||||||||||||||||||||
| iteration, slot_idx, pool->num_slots(), slot_addresses.size()); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Build remapped buffer allocations: all pooled buffers use pool VA | ||||||||||||||||||||||||||||||
| // addresses; constants and non-command-buffer buffers keep BFC addresses. | ||||||||||||||||||||||||||||||
| // For params/live-out, copy data from BFC into pool VA before execution. | ||||||||||||||||||||||||||||||
| std::vector<se::DeviceAddressBase> mapped_buffers; | ||||||||||||||||||||||||||||||
| mapped_buffers.reserve(buffer_allocations.size()); | ||||||||||||||||||||||||||||||
| int slot_addr_idx = 0; | ||||||||||||||||||||||||||||||
| for (BufferAllocation::Index i = 0; | ||||||||||||||||||||||||||||||
| i < static_cast<BufferAllocation::Index>(buffer_allocations.size()); | ||||||||||||||||||||||||||||||
| ++i) { | ||||||||||||||||||||||||||||||
| if (pool_indexes.contains(i)) { | ||||||||||||||||||||||||||||||
| auto pool_addr = slot_addresses[slot_addr_idx++]; | ||||||||||||||||||||||||||||||
| auto bfc_addr = buffer_allocations.GetDeviceAddress(i); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Copy param data from BFC into pool before execution. This is needed | ||||||||||||||||||||||||||||||
| // because the graph uses stable pool VA addresses, but the actual data | ||||||||||||||||||||||||||||||
| // lives at BFC addresses which may change. For params, the data itself | ||||||||||||||||||||||||||||||
| // may also change (e.g., optimizer weight updates in training). | ||||||||||||||||||||||||||||||
| if (copy_indexes.contains(i) && !bfc_addr.is_null() && | ||||||||||||||||||||||||||||||
| bfc_addr.size() > 0) { | ||||||||||||||||||||||||||||||
| se::DeviceAddressBase pool_dst(pool_addr.opaque(), bfc_addr.size()); | ||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(run_options->stream()->MemcpyD2D( | ||||||||||||||||||||||||||||||
| &pool_dst, bfc_addr, bfc_addr.size())); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| mapped_buffers.push_back(pool_addr); | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| mapped_buffers.push_back(buffer_allocations.GetDeviceAddress(i)); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| BufferAllocations remapped_buffer_allocations( | ||||||||||||||||||||||||||||||
| mapped_buffers, buffer_allocations.device_ordinal(), | ||||||||||||||||||||||||||||||
| buffer_allocations.memory_allocator()); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(ExecuteThunksImpl( | ||||||||||||||||||||||||||||||
| has_module() ? &module_config().debug_options() : nullptr, module_name_, | ||||||||||||||||||||||||||||||
| unique_id, *thunk_executor_, executable_source, run_options, | ||||||||||||||||||||||||||||||
| remapped_buffer_allocations, block_host_until_done, | ||||||||||||||||||||||||||||||
| execution_stream_ids_, collective_memory_cache_)); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Copy live-out results back from pool to BFC so the output appears at | ||||||||||||||||||||||||||||||
| // the expected BFC address for downstream consumers. | ||||||||||||||||||||||||||||||
| slot_addr_idx = 0; | ||||||||||||||||||||||||||||||
| for (BufferAllocation::Index i = 0; | ||||||||||||||||||||||||||||||
| i < static_cast<BufferAllocation::Index>(buffer_allocations.size()); | ||||||||||||||||||||||||||||||
| ++i) { | ||||||||||||||||||||||||||||||
| if (pool_indexes.contains(i)) { | ||||||||||||||||||||||||||||||
| auto pool_addr = slot_addresses[slot_addr_idx++]; | ||||||||||||||||||||||||||||||
| if (copy_indexes.contains(i) && buffer_assignment_) { | ||||||||||||||||||||||||||||||
| const auto& alloc = buffer_assignment_->GetAllocation(i); | ||||||||||||||||||||||||||||||
| if (alloc.maybe_live_out()) { | ||||||||||||||||||||||||||||||
| auto bfc_addr = buffer_allocations.GetDeviceAddress(i); | ||||||||||||||||||||||||||||||
| if (!bfc_addr.is_null() && bfc_addr.size() > 0) { | ||||||||||||||||||||||||||||||
| se::DeviceAddressBase bfc_dst(bfc_addr.opaque(), bfc_addr.size()); | ||||||||||||||||||||||||||||||
| se::DeviceAddressBase pool_src(pool_addr.opaque(), bfc_addr.size()); | ||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(run_options->stream()->MemcpyD2D( | ||||||||||||||||||||||||||||||
| &bfc_dst, pool_src, bfc_addr.size())); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential bug: live-out copy-back not synchronized when If Consider adding a stream synchronization after the copy-back when if (block_host_until_done) {
TF_RETURN_IF_ERROR(run_options->stream()->BlockHostUntilDone());
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved -- addressed in this revision. The copy-back D2D memcpy calls now happen before BlockHostUntilDone, so the sync correctly covers both the thunk execution and the copy-back. |
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (block_host_until_done) { | ||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(run_options->stream()->BlockHostUntilDone()); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // GPU signals slot completion so the CPU knows when this slot is safe to | ||||||||||||||||||||||||||||||
| // reuse (non-blocking write via hipStreamWriteValue64). | ||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(pool->ReleaseSlot(run_options->stream(), iteration)); | ||||||||||||||||||||||||||||||
|
Comment on lines
+1666
to
+1672
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Medium] Consider swapping the order so
Suggested change
This ensures the timeline signal is included in the synchronization fence. |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return absl::OkStatus(); | ||||||||||||||||||||||||||||||
| #else | ||||||||||||||||||||||||||||||
| return absl::UnimplementedError( | ||||||||||||||||||||||||||||||
| "Circular VMM pool is only supported on ROCm."); | ||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| absl::Status GpuExecutable::ExecuteThunks( | ||||||||||||||||||||||||||||||
| const BufferAllocations& buffer_allocations, | ||||||||||||||||||||||||||||||
| const ServiceExecutableRunOptions* run_options) { | ||||||||||||||||||||||||||||||
|
|
@@ -1587,21 +1744,32 @@ absl::Status GpuExecutable::ExecuteThunks( | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| se::StreamExecutor* executor = run_options->stream()->parent(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| bool has_cmd_buffer_allocs = !command_buffer_allocation_indexes_.empty(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Check if circular VMM pool is enabled (takes priority over VA remapping). | ||||||||||||||||||||||||||||||
| bool enable_circular_vmm_pool = | ||||||||||||||||||||||||||||||
| has_cmd_buffer_allocs && has_module() && | ||||||||||||||||||||||||||||||
| module_config().debug_options().xla_gpu_enable_circular_vmm_pool(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Check if command buffer VA remapping is enabled. | ||||||||||||||||||||||||||||||
| bool enable_command_buffer_va_remapping = | ||||||||||||||||||||||||||||||
| (command_buffer_allocation_indexes_.size() > 0) && has_module() && | ||||||||||||||||||||||||||||||
| !enable_circular_vmm_pool && has_cmd_buffer_allocs && has_module() && | ||||||||||||||||||||||||||||||
| module_config() | ||||||||||||||||||||||||||||||
| .debug_options() | ||||||||||||||||||||||||||||||
| .xla_gpu_enable_command_buffer_va_remapping() && | ||||||||||||||||||||||||||||||
| dynamic_cast<se::DeviceAddressVmmAllocator*>(memory_allocator) != nullptr; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| XLA_VLOG_DEVICE(3, executor->device_ordinal()) << absl::StreamFormat( | ||||||||||||||||||||||||||||||
| "ExecuteThunks: command_buffer_allocation_indexes_.size()=%d " | ||||||||||||||||||||||||||||||
| "enable_command_buffer_va_remapping=%d", | ||||||||||||||||||||||||||||||
| command_buffer_allocation_indexes_.size(), | ||||||||||||||||||||||||||||||
| "ExecuteThunks: cmd_buffer_allocs=%d circular_vmm_pool=%d " | ||||||||||||||||||||||||||||||
| "va_remapping=%d", | ||||||||||||||||||||||||||||||
| command_buffer_allocation_indexes_.size(), enable_circular_vmm_pool, | ||||||||||||||||||||||||||||||
| enable_command_buffer_va_remapping); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (enable_command_buffer_va_remapping) { | ||||||||||||||||||||||||||||||
| if (enable_circular_vmm_pool) { | ||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(ExecuteThunksWithCircularVmmPool( | ||||||||||||||||||||||||||||||
| buffer_allocations, run_options, executor, unique_id, executable_source, | ||||||||||||||||||||||||||||||
| block_host_until_done)); | ||||||||||||||||||||||||||||||
| } else if (enable_command_buffer_va_remapping) { | ||||||||||||||||||||||||||||||
| TF_RETURN_IF_ERROR(ExecuteThunksWithVaRemapping( | ||||||||||||||||||||||||||||||
| buffer_allocations, run_options, executor, unique_id, executable_source, | ||||||||||||||||||||||||||||||
| block_host_until_done)); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ limitations under the License. | |
| #ifndef XLA_SERVICE_GPU_GPU_EXECUTABLE_H_ | ||
| #define XLA_SERVICE_GPU_GPU_EXECUTABLE_H_ | ||
|
|
||
| #include <atomic> | ||
| #include <cstdint> | ||
| #include <deque> | ||
| #include <memory> | ||
|
|
@@ -430,6 +431,27 @@ class GpuExecutable : public Executable { | |
| absl::node_hash_map<stream_executor::StreamExecutor*, VaRanges> | ||
| module_va_ranges_ ABSL_GUARDED_BY(va_ranges_mutex_); | ||
|
|
||
| // Circular VMM pool: pre-allocated slots with permanent VA mappings and | ||
| // GPU timeline signaling for safe slot reuse. ROCm-only. | ||
| absl::Status ExecuteThunksWithCircularVmmPool( | ||
| const BufferAllocations& buffer_allocations, | ||
| const ServiceExecutableRunOptions* run_options, | ||
| stream_executor::StreamExecutor* executor, int64_t unique_id, | ||
| Thunk::ExecutableSource executable_source, bool block_host_until_done); | ||
|
|
||
| struct CircularPoolState { | ||
| std::shared_ptr<void> pool; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This works correctly because Since the header already has a |
||
| std::atomic<uint64_t> iteration_count{0}; | ||
| absl::Mutex mu; | ||
| // Cached buffer classification (computed once at init, reused every iter). | ||
| absl::btree_set<BufferAllocation::Index> pool_indexes; | ||
| absl::btree_set<BufferAllocation::Index> copy_indexes; | ||
| bool indexes_cached = false; | ||
| }; | ||
| absl::Mutex circular_pool_mutex_; | ||
| absl::node_hash_map<stream_executor::StreamExecutor*, CircularPoolState> | ||
| circular_pools_ ABSL_GUARDED_BY(circular_pool_mutex_); | ||
|
|
||
| GpuExecutable(const GpuExecutable&) = delete; | ||
| GpuExecutable& operator=(const GpuExecutable&) = delete; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: default value / help text mismatch
The actual default is set to
1(line 507:set_xla_gpu_circular_vmm_pool_slots(1)), but the flag help text here says"(default 2)".With 1 slot,
AcquireNextSlotwill spin-wait for GPU completion on every iteration (no overlap), negating the primary benefit of the circular pool. Either fix the help text to say "default 1" or change the default to 2.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially resolved. The flag help text in debug_options_flags.cc now correctly says "(default 1)", but the proto comment in xla.proto still says "(default 2)" -- see
xla_gpu_circular_vmm_pool_slotsfield comment. Please update the proto comment to match the actual default of 1.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved -- fully addressed in this revision. Both the flag help text in debug_options_flags.cc and the proto comment in xla.proto now correctly say "(default 1)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved: both the flag help text (line 3034) and proto comment now correctly say "(default 1)", matching the actual default value.