[offload] add parameters to the tests by EuphoricThinking · Pull Request #5 · EuphoricThinking/llvm-project

EuphoricThinking · 2026-06-30T10:24:03Z

tests up to two parameters are not parametrized
wondering whether to change arrays in Properties.hpp to PropertiesContainer for consistency

Split into several commits In order to enable comparing different versions.

=== Commit love letter ===
This patch adds:

a new fixture that allows for creating parameterized tests
new macros for the instantiation of parameterized tests
a helper header file that contains information related to parameterized tests, including the definition of parameters
new and refactored printers used in test instantiation

Moreover, selected existing tests are modified and parametrized in order to make the code less repetitive and more concise. Tests with up to two possible parameters are not parameterized.

Previously, the tests were already parameterized with TestEnvironment::Device. However, this patch combines TestEnvironment::Device with an additional parameter within OffloadParam = std::tuple<TestEnvironment::Device, T>. OffloadParam<T> is handled by a new fixture OffloadDeviceTestWithParam<T>, which provides the getTestParam() method for the parameter extraction. Other functionalities, such as access to the Host and to the Device, are identical to the previous version of OffloadDeviceTest.

In order to avoid code duplication, the unparameterized versions of fixtures are aliases for parameterized fixtures, with int type chosen arbitrarily as an ignored parameter type, for example: using OffloadDeviceTest = OffloadDeviceTestWithParam<int>;. The single mock parameter of value 0 is combined with the devices in the provided macros, yielding tuples of type std::tuple<TestEnvironment::Device, int>. The hidden int parameter is not used, but it enables users to instantiate unparameterized tests without knowledge about the implementation details. Moreover, it allows for modifying only one version of the fixture (the one being aliased), without the need to also change the other version: either parameterized or unparameterized.

The following macros are added in this patch:

OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE_WITH_PARAM(FIXTURE, VALUES, PRINTER): instantiates FIXTURE with parameters stored in the provided container VALUES (a C-style array or an STL-style container) and with the given printer. The used devices do not include the host.
OFFLOAD_TESTS_INSTANTIATE_HOST_DEVICE_FIXTURE_WITH_PARAM(FIXTURE, VALUES, PRINTER): similar to OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE_WITH_PARAM, but includes the Host in the tested devices.
OFFLOAD_TESTS_INSTANTIATE_HOST_DEVICE_FIXTURE(FIXTURE): instantiates FIXTURE without additional parameters, but also includes the Host in the used devices.
OFFLOAD_TESTS_INSTANTIATE_WITH_DEVICES(FIXTURE, DEVICES): instantiates FIXTURE with the provided devices, internally using a mock parameter. It is not intended for direct use and acts as a helper macro for the instantiation of unparameterized tests.
OFFLOAD_TESTS_INSTANTIATE_WITH_DEVICES_WITH_PARAM(FIXTURE, VALUES, DEVICES, PRINTER): instantiates FIXTURE with the provided devices, parameters and the given printer. It is not intended for direct use and acts as a helper macro for the instantiation of parameterized tests.

Since the new macros enable testing the Host as part of the used devices, offload/unittests/OffloadAPI/device/olGetHostInfo.cpp is deleted and its tests have been moved to offload/unittests/OffloadAPI/device/olGetDeviceInfo.cpp.

The new helper header offload/unittests/OffloadAPI/common/Properties.hpp with its implementation offload/unittests/OffloadAPI/common/Properties.cpp:

pairs properties with their sizes for tests similar to offload/unittests/OffloadAPI/device/olGetDeviceInfo.cpp
packs selected properties into containers or arrays for easier access and instantiation of fixtures

…in one go

311Volt

minor readability concerns, tests look ok to me

311Volt · 2026-07-01T08:58:12Z

+#include "Fixtures.hpp"
+#include "OffloadAPI.h"
+
+constexpr size_t MAX_DEVICE_INFO_BYTES = 8;


inline constexpr

311Volt · 2026-07-01T09:58:35Z

+constexpr char zeroArray[MAX_DEVICE_INFO_BYTES] = {};
+
+template <typename T> using PropertiesVec = std::vector<T>;
+template <typename T> using PropertyTuple = std::tuple<size_t, T>;


template<typename T> struct SizedProperty { size_t size; T prop; };

would be more readable & remove the need for getSize()/getProp()

311Volt · 2026-07-01T09:59:55Z

+
+template <typename T> using PropertiesSet = std::set<T>;
+template <typename T>
+using PropertiesTypes = std::unordered_map<T, PropertyTuple<T>>;


all of these type aliases are fairly trivial (and their names do not carry additional semantics), so they mostly serve to hide the real type from the reader - I would remove them

I wouldn't remove all of the aliasing, since in case of a change, it's easier to modify the content hidden behind the alias rather than replace all occurrences of the previous type. Instead, I have reduced the amount of aliasing and made selected names more generalized. Let me know if you have any remaining concerns about the naming convention or aliasing.

311Volt · 2026-07-01T11:38:23Z

+OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE_WITH_PARAM(
+    olMemAllocAlignedTypesTest, AllocTypes,
+    defaultPrinterWithParam<ol_alloc_type_t>);
+
 constexpr size_t DefaultAlignment = 16;
 constexpr size_t TestAllocsNum = 1000;

 TEST_P(olMemAllocAlignedTest, SuccessAllocMany) {
  std::vector<void *> Allocs;
  Allocs.reserve(1000);


.reserve(TestAllocsNum)

lplewa · 2026-07-01T15:15:20Z

+DeviceInfoProp PropCapabilitiesFlags{OL_DEVICE_INFO_SINGLE_FP_CONFIG,
+                                     OL_DEVICE_INFO_HALF_FP_CONFIG,
+                                     OL_DEVICE_INFO_DOUBLE_FP_CONFIG};
+// sizeof(ol_device_fp_capability_flags_t) == sizegof(uint32_t)


siegof?.

maybe static assert if you need check like this

It's not needed, I have thought that an informative reminder in a comment would be useful

311Volt · 2026-07-07T07:48:50Z

@@ -38,12 +29,9 @@ TEST_P(olMemAllocTest, SuccessAllocMany) {
  std::vector<void *> Allocs;
  Allocs.reserve(1000);


num of allocs not extracted to constant (inconsistent with olMemAllocAlignedTypesTest)

Good catch :) I have moved the definition of TestAllocsNum to the common header file

311Volt · 2026-07-07T07:51:25Z

+
+inline constexpr size_t MAX_DEVICE_INFO_BYTES = 8;
+
+constexpr char zeroArray[MAX_DEVICE_INFO_BYTES] = {};


inline constexpr again

311Volt · 2026-07-07T07:51:36Z

+#include "Fixtures.hpp"
+#include "OffloadAPI.h"
+
+inline constexpr size_t MAX_DEVICE_INFO_BYTES = 8;


MACRO_CASE should be reserved for macros

at least LLVM coding standards imply as much, but I can see that e.g. liboffload enumerators are all MACRO_CASE anyway sooo ¯\_(ツ)_/¯

I see... I would leave it as it is, for the sake of liboffload spelling convention consistency.

311Volt · 2026-07-07T08:34:15Z

-}
-
-TEST_P(olMemAllocAlignedTest, SuccessAllocHostDifferentAlignments) {
+TEST_P(olMemAllocAlignedTypesTest, SuccessAllocDifferentAlignments) {
  void *Alloc = nullptr;
  size_t NumAlignments = 6;


can be replaced with sizeof(Alignments)/sizeof(Alignments[0]) to avoid surprises in the future, or use std::array

311Volt · 2026-07-07T08:37:10Z

-}
-
-TEST_P(olMemAllocAlignedTest, SuccessAllocHostDifferentAlignments) {
+TEST_P(olMemAllocAlignedTypesTest, SuccessAllocDifferentAlignments) {
  void *Alloc = nullptr;
  size_t NumAlignments = 6;
  size_t Alignments[] = {8, 16, 32, 64, 128, 256};


are all plugins specified to support this set of values? imo ErrorCode::UNSUPPORTED should not be a fail

this is pre-existing, may be outside the scope of this PR

That's a good question, but I'll leave it for another discussion

311Volt · 2026-07-07T08:43:25Z

+// Moreover, it allows for modifying only one version of the fixture, without
+// the need to also change the other version: either parameterized or
+// unparameterized.
+using OffloadDeviceTest = OffloadDeviceTestWithParam<int>;


maybe std::monostate or struct UnusedTestParam {}; could be used here to reduce risk of confusion? (nitpick bc it's mostly opaque to users of INSTANTIATE macros)

I'm curious about what your vision is of how the proposed structures should be used here. It's also been a long time since I've worked with std::variant, so I would rely on your expertise. However, even if that might be a "cleaner" approach, I'm not sure whether this would add complexity to the code or whether it is just a matter of preference.

…es; Macros are also Substituted with helpers

EuphoricThinking changed the title ~~Parametrized tests1~~ [offload] add parameters tests Jul 1, 2026

EuphoricThinking changed the title ~~[offload] add parameters tests~~ [offload] add parameters to the tests Jul 1, 2026

EuphoricThinking force-pushed the parametrized_tests1 branch 2 times, most recently from 42efb66 to 8955f05 Compare July 1, 2026 08:36

I've deleted commit messages so that it looks as if it has been made …

8955f05

…in one go

311Volt reviewed Jul 1, 2026

View reviewed changes

lplewa reviewed Jul 1, 2026

View reviewed changes

EuphoricThinking force-pushed the parametrized_tests1 branch from c8734f1 to b261bd6 Compare July 3, 2026 18:15

Changed Properties.hpp so that it uses SizedProperty

b261bd6

EuphoricThinking force-pushed the parametrized_tests1 branch 4 times, most recently from aabb1d4 to 450a391 Compare July 4, 2026 16:24

311Volt reviewed Jul 7, 2026

View reviewed changes

Fixtures are mocking a parameter in fixtures without parameter fixtur…

9600346

…es; Macros are also Substituted with helpers

EuphoricThinking force-pushed the parametrized_tests1 branch from 450a391 to 9600346 Compare July 8, 2026 16:50

		@@ -38,12 +29,9 @@ TEST_P(olMemAllocTest, SuccessAllocMany) {
		std::vector<void *> Allocs;
		Allocs.reserve(1000);


		inline constexpr size_t MAX_DEVICE_INFO_BYTES = 8;

		constexpr char zeroArray[MAX_DEVICE_INFO_BYTES] = {};

Conversation

EuphoricThinking commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

311Volt 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!

311Volt Jul 1, 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!

311Volt Jul 1, 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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EuphoricThinking commented Jun 30, 2026 •

edited

Loading

311Volt Jul 1, 2026 •

edited

Loading

311Volt Jul 1, 2026 •

edited

Loading