[SYCL][E2E] refactor: drop vulkan_common dependencies from bindless image tests by mianowski · Pull Request #23047 · intel/llvm

mianowski · 2026-08-28T08:05:51Z

#22885

KornevNikita

CODEOWNERS change LGTM.

mianowski · 2026-08-31T07:43:48Z

@dyniols please review

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

mianowski · 2026-09-02T10:53:21Z

+inline VkImageMemoryBarrier createImageMemoryBarrier(VkImage image,
+                                                     uint32_t mipLevels) {
+  VkImageMemoryBarrier barrier{VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER};
+  barrier.oldLayout = VK_IMAGE_LAYOUT_UNDEFINED;
+  barrier.newLayout = VK_IMAGE_LAYOUT_GENERAL;
+  barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
+  barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
+  barrier.image = image;
+  barrier.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT;
+  barrier.subresourceRange.levelCount = mipLevels;
+  barrier.subresourceRange.layerCount = 1;
+  return barrier;
+}


mianowski · 2026-09-02T10:57:18Z

 int main() {
-
-  if (vkutil::setupInstance() != VK_SUCCESS) {
-    std::cerr << "Instance setup failed!\n";
-    return EXIT_FAILURE;
-  }
-
  sycl::device dev;
-
-  if (vkutil::setupDevice(dev) != VK_SUCCESS) {
-    std::cerr << "Device setup failed!\n";
-    return EXIT_FAILURE;
-  }
-
-  if (vkutil::setupCommandBuffers() != VK_SUCCESS) {
-    std::cerr << "Compute pipeline setup failed!\n";
-    return EXIT_FAILURE;
-  }
-
-  bool result_ok = run_tests();
-
-  if (vkutil::cleanup() != VK_SUCCESS) {
-    std::cerr << "Cleanup failed!\n";
-    return EXIT_FAILURE;
-  }
+  VulkanContext vkCtx = createVulkanContext();
+  bool result_ok = run_tests(vkCtx);
+  cleanupVulkanContext(vkCtx);


mianowski · 2026-09-03T08:32:48Z

+  VulkanContext vkCtx = createVulkanContext();
+  auto testPassed = runTest(vkCtx, syclDevice, {128, 128}, {16, 16});
+  cleanupVulkanContext(vkCtx);



mianowski · 2026-09-02T11:01:50Z

+    VkCommandPoolCreateInfo PoolInfo = {
+        VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO};
+    PoolInfo.queueFamilyIndex = VulkanCtx.queueFamilyIndex;
+    VkCommandPool Pool;
+    VK_CHECK(vkCreateCommandPool(VulkanCtx.device, &PoolInfo, nullptr, &Pool));
+    VkCommandBufferAllocateInfo CmdAllocInfo = {
+        VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO};
+    CmdAllocInfo.commandPool = Pool;
+    CmdAllocInfo.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY;
+    CmdAllocInfo.commandBufferCount = 1;
+    VkCommandBuffer CommandBuffer;
+    VK_CHECK(vkAllocateCommandBuffers(VulkanCtx.device, &CmdAllocInfo,
+                                      &CommandBuffer));
+    VK_CHECK(vkBeginCommandBuffer(CommandBuffer, &Cbbi));
+    vkCmdCopyBuffer(CommandBuffer, VkImportedBuffer, StagingBuffer,
                    1 /*regionCount*/, &CopyRegion);
-    VK_CHECK_CALL(vkEndCommandBuffer(vk_transferCmdBuffers[0]));
+    VK_CHECK(vkEndCommandBuffer(CommandBuffer));

    std::vector<VkPipelineStageFlags> Stages{VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT};

    VkSubmitInfo Submission = {};
    Submission.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO;
    Submission.commandBufferCount = 1;
-    Submission.pCommandBuffers = &vk_transferCmdBuffers[0];
+    Submission.pCommandBuffers = &CommandBuffer;
    Submission.pWaitDstStageMask = Stages.data();

-    VK_CHECK_CALL(vkQueueSubmit(vk_transfer_queue, 1 /*submitCount*/,
-                                &Submission, VK_NULL_HANDLE /*fence*/));
-    VK_CHECK_CALL(vkQueueWaitIdle(vk_transfer_queue));
+    VK_CHECK(vkQueueSubmit(VulkanCtx.queue, 1, &Submission, VK_NULL_HANDLE));
+    VK_CHECK(vkQueueWaitIdle(VulkanCtx.queue));


cperkinsintel · 2026-09-01T22:12:13Z

    // Use helper function to determine if data is accepted.
    // For floats, use default accepted error variance.
-    if (!util::is_equal(outputVec[i], expected)) {
+    if (std::abs(outputVec[i] - expected) > 0.01f) {


maybe just bring over that utility routine? Or define a new one (occassionally_equal) ;-) ?

dyniols · 2026-09-01T15:03:30Z

I think we can remove the CommonUtils directory, as no other files will remain once vulkan_common.hpp is removed.

mianowski · 2026-09-03T08:48:31Z

I think we can remove the CommonUtils directory, as no other files will remain once vulkan_common.hpp is removed.

Git doesn't store folders, only files. So, once the last file from a folder is gone, the folder vanishes too

Copilot

🟡 Changes recommended

Device selection can pair different Vulkan and SYCL GPUs, and one migrated test lacks exception-safe cleanup.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 6/6 changed files
Comments generated: 4
Review effort level: Balanced

dyniols · 2026-09-03T10:35:20Z

-    std::cerr << "Instance setup failed!\n";
+  VulkanContext VulkanCtx;
+  try {
+    VulkanCtx = createVulkanContext();


I think we should restore UUID based selection in vulkan_setup.hpp. Specifically, instantiate the SYCL device once, pass it to createVulkanContext, and select the VkPhysicalDevice matching the SYCL device UUID.

However it doesn't have to be done in this PR because it will affect other vulkan interop tests and it could be done in another PR.

Your thoughts @cperkinsintel @0x12CC ?

-    std::cerr << "Cleanup failed!\n";
-    return 7;
-  }
+  auto TestPassed = runTest(VulkanCtx, SyclDevice, MemorySizeBytes);


Copilot

🟡 Changes recommended

Device matching reads an uninitialized handle, and SYCL cleanup failures can incorrectly produce a passing result.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 6/6 changed files
Comments generated: 2
Review effort level: Balanced

+  auto SyclDeviceUUID =
+      SyclDevice.get_info<sycl::ext::intel::info::device::uuid>();
+  for (VkPhysicalDevice device : devices) {


+      } catch (const sycl::exception &e) {
+        std::cerr << "SYCL cleanup failed: " << e.what() << "\n";
+      } catch (...) {
+        std::cerr << "Unknown exception during SYCL cleanup.\n";
+      }


dyniols

Overall looks great! Left a few minor comments and questions.

dyniols · 2026-09-03T12:06:45Z

  vkEnumeratePhysicalDevices(ctx.instance, &deviceCount, devices.data());
-  ctx.physicalDevice = devices[0];
+
+  if (!SyclDevice.has(sycl::aspect::ext_intel_device_info_uuid))


Can we avoid bringing SYCL code into vulkan_setup.hpp?

For example we could pass SYCL device UUID rather than pass SYCL device to here:
https://github.com/triSYCL/sycl/blob/sycl/unified/master/sycl/doc/extensions/supported/sycl_ext_intel_device_info.md#device-uuid

dyniols · 2026-09-03T12:15:20Z

+  for (VkPhysicalDevice device : devices) {
+    VkPhysicalDeviceIDProperties idProperties{
+        VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_ID_PROPERTIES};
+    VkPhysicalDeviceProperties2 properties{


Consider zero initializing the Vulkan structures to avoid uninitialized member fields.

dyniols · 2026-09-03T12:19:54Z

  vkFreeMemory(ctx.device, res.memory, nullptr);
 }

+inline VkCommandBuffer createCommandBuffer(VulkanContext &ctx,


Consider using const VulkanContext &ctx if ctx isn't modified here.

dyniols · 2026-09-03T12:20:19Z


+inline VkCommandBuffer createCommandBuffer(VulkanContext &ctx,
+                                           VkCommandPool &pool) {
+  VkCommandPoolCreateInfo poolInfo{VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO};


Same here zero initializing the Vulkan structures.

dyniols · 2026-09-03T12:26:36Z

+  vkDestroyCommandPool(ctx.device, pool, nullptr);
+}
+
+inline VkImageMemoryBarrier createImageMemoryBarrier(VkImage image,


If this is intended as a generic helper, should we expose VkImageLayout and VkAccessFlags as function parameters to make it more flexible?

dyniols · 2026-09-03T12:31:37Z

I know it's old change but we could do const sycl::exception &e.

dyniols · 2026-09-03T13:02:04Z

Please add the [SYCL] tag to the PR title

mianowski added 5 commits August 27, 2026 11:49

Added step to verify clang-format config

9dd4359

Remove vulkan_common

426c43e

CommandBuffer

6412bd0

Remove checking formatter configuration from pipeline

c829c5e

Remove vulkan_common.hpp

8114e96

mianowski changed the title ~~[E2E] chore: drop vulkan_common from bindless image tests~~ [E2E] refactor: drop vulkan_common dependencies from bindless image tests Aug 28, 2026

char to int8_t

a6d9e27

mianowski marked this pull request as ready for review August 28, 2026 14:38

mianowski requested review from a team as code owners August 28, 2026 14:38

mianowski requested a review from 0x12CC August 28, 2026 14:38

KornevNikita requested a review from dyniols August 28, 2026 16:25

KornevNikita linked an issue Aug 28, 2026 that may be closed by this pull request

Drop CommonUtils/vulkan_common.hpp from bindless images e2e tests and migrate to vulkan_setup.hpp #22885

Open

KornevNikita approved these changes Aug 28, 2026

View reviewed changes

dyniols requested a balanced review from Copilot September 1, 2026 07:35

Copilot started reviewing on behalf of dyniols September 1, 2026 07:36 View session

Copilot AI reviewed Sep 1, 2026

dyniols requested a balanced review from Copilot September 1, 2026 11:45

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Copilot started reviewing on behalf of dyniols September 1, 2026 12:32 View session

post-review

f7941fd

mianowski requested review from cperkinsintel and a balanced review from Copilot September 3, 2026 08:49

Copilot started reviewing on behalf of mianowski September 3, 2026 08:50 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

post-review

b0d6303

dyniols requested a balanced review from Copilot September 3, 2026 12:03

Copilot started reviewing on behalf of dyniols September 3, 2026 12:03 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

dyniols reviewed Sep 3, 2026

View reviewed changes

mianowski changed the title ~~[E2E] refactor: drop vulkan_common dependencies from bindless image tests~~ [SYCL][E2E] refactor: drop vulkan_common dependencies from bindless image tests Sep 3, 2026

mianowski added 2 commits September 3, 2026 19:19

sycl_vulkan_setup in bindless image tests

bd7110b

post-review

48ed7ff

Uh oh!

Conversation

mianowski commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KornevNikita left a comment

Choose a reason for hiding this comment

Uh oh!

mianowski commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dyniols commented Sep 1, 2026

Uh oh!

mianowski commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

dyniols left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dyniols Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dyniols commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mianowski commented Aug 28, 2026 •

edited

Loading

mianowski commented Aug 31, 2026 •

edited

Loading

mianowski commented Sep 3, 2026 •

edited

Loading

dyniols Sep 3, 2026 •

edited

Loading