Skip to content

UT for update mutable command and mutable command getinfo#17

Open
Bogumil-Sapinski-Mobica wants to merge 3 commits intomainfrom
ut_for_update_mutable_commands
Open

UT for update mutable command and mutable command getinfo#17
Bogumil-Sapinski-Mobica wants to merge 3 commits intomainfrom
ut_for_update_mutable_commands

Conversation

@Bogumil-Sapinski-Mobica
Copy link
Collaborator

No description provided.

@piotr-wozniak-mobica piotr-wozniak-mobica force-pushed the ut_for_update_mutable_commands branch from 574701d to c1a201d Compare September 11, 2023 07:54
@piotr-wozniak-mobica piotr-wozniak-mobica force-pushed the ut_for_update_mutable_commands branch from c1a201d to 50a5c72 Compare September 11, 2023 07:58
@piotr-wozniak-mobica
Copy link
Collaborator

@shajder could you review this?

****************************************************************************/

#if defined(cl_khr_command_buffer_mutable_dispatch)
cl_int clUpdateMutableCommandsKHR_testCommandBufferKhrUpdateMutableCommands(
Copy link
Collaborator

Choose a reason for hiding this comment

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

static please

TEST_ASSERT_EQUAL(CL_SUCCESS, response);
}

cl_int clGetMutableCommandInfoKHR_testMutableCommandKhrGetInfo(
Copy link
Collaborator

Choose a reason for hiding this comment

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

static please

Comment on lines 3651 to 3653
(void)param_value_size;
(void)param_value;
(void)param_value_size_ret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should really verify all those parameters passed in here. Please notice it should have different size and type depending on param_name value. Moreover, I think in this specific case we should test all available parameters (please refer to spec section 50.8.3, Table 72) to make sure param_value and param_value_size have correct sizes, then cast specific type and set dummy value.

Another possible improvement of this procedure is testing param_value_size_ret variable. If that's not nullptr we should return real data.

@kamil-goras-mobica kamil-goras-mobica force-pushed the ut_for_update_mutable_commands branch from 764340d to 68702e9 Compare October 24, 2023 12:12
Copy link
Collaborator

@shajder shajder left a comment

Choose a reason for hiding this comment

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

Except those two minor comments it LGTM

Comment on lines 356 to 359
// Deactivated because unused for now.
#if 0
MAKE_REFCOUNT_STUBS(cl_mutable_command_khr, clUpdateMutableCommandsKHR, clGetMutableCommandInfoKHR)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, I would rather remove or comment out


cl::vector<cl_ndrange_kernel_command_properties_khr> kernel_properties = mutableCommandKhrPool[0].getInfo<CL_MUTABLE_DISPATCH_PROPERTIES_ARRAY_KHR>(&err);
TEST_ASSERT_EQUAL(CL_SUCCESS, err);
for (int i = 0; i < kernel_properties.size(); i++)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could TEST_ASSERT_ size of kernel_properties vector first ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shajder changes added

@kamil-goras-mobica kamil-goras-mobica force-pushed the ut_for_update_mutable_commands branch from 4d5c1e7 to 11d03cd Compare October 26, 2023 08:59
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.

4 participants