Skip to content

Conversation

slawekptak
Copy link
Contributor

The new kernel submit wrapper functions allow to select the submission function type between a standard handler-based function and queue shortcut function. This change allows to increase the test coverage and allows for testing no-handler submission path using existing unit tests.

Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, overall. Minor comments.

Comment on lines +1 to +7
//==-- CommandSubmitWrappers.hpp --- -----==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//==-- CommandSubmitWrappers.hpp --- -----==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//==-- CommandSubmitWrappers.hpp -----------------------------------------==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include <sycl/event.hpp>
#include <sycl/handler.hpp>

using namespace sycl;
Copy link
Contributor

@uditagarwal97 uditagarwal97 Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally avoid using using namespace sycl here as it will pollute the global namespace of every test that includes this header file.

#include <sycl/handler.hpp>

using namespace sycl;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please add a comment here about why we need this wrapper?

}

template <typename KernelName, typename KernelType>
event single_task_wrapper(bool ShortcutSubmitFunction, queue &Q, event DepEvent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
event single_task_wrapper(bool ShortcutSubmitFunction, queue &Q, event DepEvent,
event single_task_wrapper(bool ShortcutSubmitFunction, queue &Q, event &DepEvent,

cgh.single_task<KernelName>(KernelFunc);
});
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

// Check that isolated kernels (i.e. those that don't modify the graph)
// are handled properly during filtering.
sycl::unittest::UrMock<> Mock;
sycl::platform Plt = sycl::platform();
bool ShortcutSubmitFunction = GetParam();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I'd have used UseShortcutFunction (or something similar) variable name instead of ShortcutSubmitFunction.

@@ -190,3 +189,6 @@ TEST_F(SchedulerTest, InOrderQueueNoSchedulerPath) {
}

} // anonymous namespace

INSTANTIATE_TEST_SUITE_P(SchedulerTestInstance, SchedulerTest,
testing::Values(true, false));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
testing::Values(true, false));
testing::Values(true, false) /*Whether to use shortcut submission function or not?*/);

@aelovikov-intel aelovikov-intel removed their request for review August 26, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants