[Bug] Reuse VkInstance across init/reset cycles

[hughperkins]

The NVIDIA Vulkan driver has a bug where repeated
vkDestroyInstance/vkCreateInstance cycles corrupt
SubgroupLocalInvocationId after ~11 cycles. The corruption is permanent for the process lifetime and cannot be recovered from, even with VK_EXT_subgroup_size_control + requiredSubgroupSize=32.

Fix: keep the VkInstance alive in VulkanLoader (process-lifetime singleton) and reuse it across qd.init/qd.reset cycles. Only the VkDevice is destroyed and recreated.

Verified: 30 consecutive Vulkan init/reset cycles all produce correct SubgroupLocalInvocationId = [0..15].

Reproducer: standalone C program (tmp/vk_subgroup_repro.c) confirms the bug is in the NVIDIA driver, not in Quadrants code.

Issue: #

Brief Summary

copilot:summary

Walkthrough

copilot:walkthrough

[hughperkins]

why does VulkanLoader::instance().get_instance(); always return the same instance?

[hughperkins]

opus response:

VulkanLoader is a Meyer's singleton — the static local variable ensures there's exactly one instance per process:

quadrants/quadrants/rhi/vulkan/vulkan_loader.h lines 296-299

static VulkanLoader &instance() {
  static VulkanLoader instance;
  return instance;
}

And it holds the VkInstance as a member:

quadrants/quadrants/rhi/vulkan/vulkan_loader.h lines 44-44

VkInstance vulkan_instance_{VK_NULL_HANDLE};

The flow across qd.init()/qd.reset() cycles:

1. First qd.init(): get_instance() returns VK_NULL_HANDLE, so create_instance() creates a new VkInstance and stores it via load_instance(instance_).
2. qd.reset(): ~VulkanDeviceCreator destroys the VkDevice but no longer destroys the VkInstance. The singleton's vulkan_instance_ still points to the live handle.
3. Second qd.init(): get_instance() returns the same live VkInstance, so create_instance() reuses it and returns early.

The VulkanLoader singleton outlives all VulkanDeviceCreator instances — it's destroyed only at process exit.

[hughperkins]

why do we have both instance_ and vulkan_instance_?

[hughperkins]

opus:

They're in different classes with different lifetimes:
• VulkanDeviceCreator::instance_ — a local copy on the creator object, used during device setup (picking physical device, creating logical device, etc.). It's created per
qd.init() call and dies with the creator on qd.reset().
• VulkanLoader::vulkan_instance_ — the singleton's copy that persists for the process lifetime. It's also used by load_function() to resolve Vulkan proc addresses.

They point to the same VkInstance handle. It's redundant — instance_ could just read from the loader each time. But it's existing code structure: VulkanDeviceCreator was
originally designed to own and destroy its VkInstance, so it kept its own copy. Our fix just made it stop destroying it and start reusing the loader's copy on subsequent
cycles.
Cleaning it up (removing instance_ and always going through the loader) would be a nice follow-up refactor, but not strictly necessary for the fix.

[hughperkins]

how much code change would be required to clean up instance_ ? are there any perofrmanace implications?

[hughperkins]

Opus:

11 uses of instance_ in the file. To remove it you'd:

1. Remove VkInstance instance_ from the header (1 line)
2. Replace all 11 occurrences with VulkanLoader::instance().get_instance() (or cache it in a local variable at the top of each method)

That's ~15 lines changed. No performance implications — VulkanLoader::instance() is a static local reference (zero-cost after first call), and get_instance() is an inline
getter returning a member. These are all called during init/teardown, never in hot paths.
It's a trivial cleanup but touches many lines for no functional change, so it's a judgment call whether it's worth the diff noise.

[hughperkins]

lets leave it

[hughperkins]

note to self: method VulkanDeviceCreator::~VulkanDeviceCreator()

[hughperkins]

what happens if existing is null?

[hughperkins]

Opus:

If it's null, we fall through to the original code below that creates a new VkInstance via vkCreateInstance(...) and stores it in the loader via
VulkanLoader::instance().load_instance(instance_) at line 419.
So it's just the first-call vs subsequent-call path:
• First qd.init(): existing == VK_NULL_HANDLE → create new VkInstance, store in loader
• Subsequent qd.init(): existing != VK_NULL_HANDLE → reuse it, return early

[hughperkins]

what si the purpose of this code?

[hughperkins]

Opus:

That's in VulkanLoader::check_vulkan_device() — a probe function that runs once during VulkanLoader::init() to check if there's a GPU with a compute queue. It creates a
temporary VkInstance just to enumerate physical devices, then destroys it.
The problem was that load_instance(instance) on line 46 stored the temporary handle into vulkan_instance_, and then line 80 destroyed it — leaving vulkan_instance_ as a
dangling pointer. Without the vulkan_instance_ = VK_NULL_HANDLE fix, our reuse check in create_instance() would see a non-null handle and try to reuse a destroyed instance.
So that one line I added just ensures the probe cleanup doesn't leave a stale pointer that would fool the reuse logic.

[hughperkins]

to Opus: does this pr have any tests?

[hughperkins]

Opus:

No, the branch has no committed tests. The test I ran (tmp/test_vulkan_instance_reuse.py) was ad-hoc and not committed.
Want me to add a proper test to the branch? It would cycle qd.init(vulkan)/qd.reset() ~20 times and verify SubgroupLocalInvocationId stays correct — essentially the same thing
but as a pytest in the test suite.

[hughperkins]

I have read every line added in this PR, and reviewed the lines. I take responsibilty for the lines added and removed in this PR, and won't blame any issues on Opus.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3819b64d6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve validation-layer settings when reusing VkInstance

This early return skips all instance creation options, so after one non-debug Vulkan init, later qd.init(..., debug=True) runs (wired via VulkanProgramImpl::materialize_runtime) silently reuse an instance that was created without validation layers. That means debug-mode validation and debug printf behavior are no longer actually enabled after reset/reinit cycles, even though callers explicitly requested them, which can hide correctness issues and make debug behavior depend on init order.

Useful? React with 👍 / 👎.

[claude]

🔴 The early-return path added in create_instance() (lines 304-307) only sets instance_ and vk_api_version on the freshly-constructed VulkanDevice, but never sets physical_device_features2 (or surface). Because ti_device_ is rebuilt as std::make_unique() on every qd.init() call, those flags default to false on the 2nd+ cycle. In create_logical_device() the entire if (ti_device_->vk_caps().physical_device_features2) block is the only place that queries VkPhysicalDeviceFeatures2 and enables spirv_has_variable_ptr, spirv_has_float16, spirv_has_int8, spirv_has_atomic_float_add, spirv_has_shader_clock, buffer device address, and 8/16-bit storage — it is skipped entirely on every reinit. Fix: in the early-return path also propagate physical_device_features2 (and surface) from the first-cycle detection, or unconditionally set physical_device_features2 = true since the extension is already loaded into the live VkInstance.

Extended reasoning...

Root cause

create_instance() detects the VK_KHR_get_physical_device_properties2 instance extension inside the vkEnumerateInstanceExtensionProperties loop and sets ti_device_->vk_caps().physical_device_features2 = true (line 381). The new early-return path (lines 304-307) exits the function before that loop, so on the 2nd+ qd.init() call the flag stays at its default value of false. The same loop is also the only place ti_device_->vk_caps().surface is set (line 378), so that cap is silently absent on reinit as well.

Why the freshly-constructed VulkanDevice does not help

VulkanDeviceCreator creates ti_device_ = std::make_unique() in its constructor on every qd.init() cycle. That new VulkanDevice starts with a zero-initialized VulkanCapabilities struct; there is no copy from any previous cycle. So every reinit begins with physical_device_features2 = false regardless of what was detected before.

Code path that is silently skipped

In create_logical_device() at line 724:

if (ti_device_->vk_caps().physical_device_features2) {
// VkPhysicalDeviceFeatures2KHR query block — sole call site
// Sets: spirv_has_variable_ptr, spirv_has_atomic_float_add,
// spirv_has_atomic_float, spirv_has_atomic_float64*,
// spirv_has_float16, spirv_has_int8, spirv_has_atomic_float16*,
// spirv_has_shader_clock, buffer device address,
// 8-bit / 16-bit storage features
}

If this block is skipped, all those DeviceCapability flags remain unset even though the VkDevice is created with those extensions enabled.

Concrete step-by-step proof

1. First qd.init(vulkan): get_instance() returns VK_NULL_HANDLE, falls through to the full path, enumeration loop runs, physical_device_features2 = true, create_logical_device() sees true, all caps populated correctly.
2. qd.reset(): VulkanDeviceCreator is destroyed; VkDevice is destroyed; VkInstance is kept alive in the singleton.
3. Second qd.init(vulkan): new VulkanDeviceCreator, new VulkanDevice (physical_device_features2 = false), get_instance() returns the live handle, early return fires, physical_device_features2 is never set, create_logical_device() sees false, entire Features2 block is skipped, spirv_has_float16, spirv_has_int8, spirv_has_atomic_float_add, spirv_has_variable_ptr, spirv_has_shader_clock all remain false.

Impact

Every qd.init() after the first qd.reset() silently degrades the device capability set, excluding float16, int8, atomic float, variable pointers, shader clock, and buffer device address even though the GPU and driver fully support them.

Fix

In the early-return path, set physical_device_features2 = true unconditionally (the extension is already baked into the live VkInstance) and set surface similarly. Alternatively, cache the first-cycle caps in the VulkanLoader singleton and copy them on reuse.

[hughperkins]

walk me through what happens in create_instance

[hughperkins]

Opus:

Here's the flow:
Phase 1 — Capability discovery (lines 300–317)
Enumerates instance extensions available on the system and sets two flags on ti_device_->vk_caps():
• surface — if VK_KHR_surface is present
• physical_device_features2 — if VK_KHR_get_physical_device_properties2 is present

This runs every cycle because ti_device_ is freshly constructed each time (all caps default to false). These flags are critical — physical_device_features2 gates whether
create_logical_device() will query and enable f16, i8, atomic float, variable pointers, shader clock, buffer device address, etc.
Phase 2 — VkInstance reuse check (lines 319–327)
Asks the VulkanLoader singleton for an existing VkInstance. If one exists (2nd+ qd.init() cycle), copies it into instance_, sets the API version, and returns early. This
avoids the NVIDIA driver bug that corrupts subgroup IDs after ~11 vkDestroyInstance/vkCreateInstance cycles.
Phase 3 — First-time VkInstance creation (lines 329–423)
Only runs on the very first qd.init():

1. App info (329–335) — standard Vulkan boilerplate
2. Validation layers (341–362) — if params_.enable_validation_layer, checks layer support and chains debug messenger into pNext
3. Debug printf (364–374) — enables VK_VALIDATION_FEATURE_ENABLE_DEBUG_PRINTF_EXT if validation is on
4. Extension set (376–393) — collects required extensions + any additional ones from params_, then adds supported well-known extensions (surface, properties2, external
   memory/semaphore, debug utils)
5. vkCreateInstance (404–405) — creates the actual instance
6. Fallback (407–414) — if the driver returns VK_ERROR_INCOMPATIBLE_DRIVER, retries with Vulkan 1.0 API version
7. Store in singleton (423) — VulkanLoader::instance().load_instance(instance_) so future cycles can reuse it

[hughperkins]

Add a doc string wiht this information

[hughperkins]

what is the relationship between ti_device_ and VkInstance?

[hughperkins]

They're at different levels of abstraction:
• VkInstance (line 82, instance_) is a raw Vulkan handle — the connection to the Vulkan runtime/driver. It's used to enumerate physical devices, create logical devices, and
resolve function pointers. There's one per process (reused via the VulkanLoader singleton).
• ti_device_ (line 91) is a std::unique_ptr — Quadrants' own wrapper that holds the logical device (VkDevice), capability flags (vk_caps()), memory allocator,
pipeline cache, etc. It's the object that the rest of the Quadrants runtime interacts with.

The relationship: VulkanDeviceCreator uses instance_ during setup to discover what the hardware supports, then populates ti_device_'s capability flags and creates the VkDevice
inside it. Once construction is done, ti_device_ is handed off (via device()) and instance_ is no longer needed by the creator.
Key lifecycle difference: ti_device_ is recreated every qd.init() cycle (fresh VulkanDevice with all caps defaulting to false), while instance_ is now reused across cycles via
the singleton. That's exactly why the capability discovery had to be moved before the early return — the instance persists but the device object doesn't.

[hughperkins]

Thanks!