Skip to content

Conversation

v-klochkov
Copy link
Contributor

Signed-off-by: Vyacheslav N Klochkov vyacheslav.n.klochkov@intel.com

@v-klochkov v-klochkov requested a review from romanovvlad May 11, 2019 04:41
Copy link
Contributor

@vladimirlaz vladimirlaz left a comment

Choose a reason for hiding this comment

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

sycl/include/CL/sycl/types.hpp also contains uint:
sycl/include/CL/sycl/types.hpp:1881:DECLARE_SYCL_VEC(uint)

and sycl/test/basic_tests/types.cpp also have:
sycl/test/basic_tests/types.cpp:48: CHECK_TYPE(uint);
and sycl/test/sub_group/sg.cl:
sycl/test/sub_group/sg.cl:10: uint local_id;
sycl/test/sub_group/sg.cl:11: uint local_range;
sycl/test/sub_group/sg.cl:12: uint max_local_range;
sycl/test/sub_group/sg.cl:13: uint group_id;
sycl/test/sub_group/sg.cl:14: uint group_range;
sycl/test/sub_group/sg.cl:15: uint uniform_group_range;
sycl/test/sub_group/sg.cl:18: uint id = get_global_id(0);

and sycl/test/struct_param/struct_kernel_param.cpp:
sycl/test/struct_param/struct_kernel_param.cpp:87: uint ice2 = 888;
sycl/test/struct_param/struct_kernel_param.cpp:88: uint result[4] = {0};

I guess we will have failures for compilation on windows as well

@v-klochkov
Copy link
Contributor Author

Vladimir, those other places where uint is used do not cause any errors on Windows.
For commands.h/cpp I had choice: to use cl::sycl::uint or use uint32_t. I chose the 2nd.

@vladimirlaz
Copy link
Contributor

Vladimir, those other places where uint is used do not cause any errors on Windows.
For commands.h/cpp I had choice: to use cl::sycl::uint or use uint32_t. I chose the 2nd.

Could you explain why we have warning in one place and do not have in the other for the similar code?

@v-klochkov
Copy link
Contributor Author

Errors, not warnings.
Initially I thought it was something wrong with namespaces, but after an additional quick investigation I realized that the alternative fix to my patch is to simply add add '#include <types.hpp>' to commands.h.

It is rather the matter of taste here (cl::sycl::uint vs uint32_t).
uint32_t is used much more often in SYCL sources/headers than 'uint'. So, I would stick to the current 'uint32_t' fix.

@vladimirlaz
Copy link
Contributor

Errors, not warnings.
Initially I thought it was something wrong with namespaces, but after an additional quick investigation I realized that the alternative fix to my patch is to simply add add '#include <types.hpp>' to commands.h.

It is rather the matter of taste here (cl::sycl::uint vs uint32_t).
uint32_t is used much more often in SYCL sources/headers than 'uint'. So, I would stick to the current 'uint32_t' fix.

Thank you for clarification. LGTM.

vladimirlaz
vladimirlaz previously approved these changes May 14, 2019
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

You fixed only forward declarations, but not definitions.
Are they use something different from uint?

@@ -92,7 +92,7 @@ void ExecuteKernelCommand<
}

if (m_KernelArgs != nullptr) {
unsigned ArgumentID = 0;
uint32_t ArgumentID = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems to be unrelated to the fix.
Suggest reverting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change IS related to the fix.
The fix changed the 1st argument of passGlobalAccessorAsArg() and passLocalAccessorAsArg from uint to uint32_t and ArgumentID is passed to there.

BTW, your comment helped me to find couple places where I did not move the patch properly from WIndows (where it was complete) to Linux (where I also tested the patch and from where uploaded fix for review). Those 2 places are fixed now (changed uint to uint32_t there).

@@ -408,11 +408,11 @@ template <int DimSrc, int DimDest> class CopyCommand : public Command {
// accessor in lambda function. 'ClKernel' is the kernel. 'HostKernel'
// is the pointer to the lambda function.
template <int AccessDimensions, typename KernelType>
uint passGlobalAccessorAsArg(uint I, int LambdaOffset, cl_kernel ClKernel,
const KernelType &HostKernel);
uint32_t passGlobalAccessorAsArg(uint I, int LambdaOffset, cl_kernel ClKernel,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to replace uint I?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you. Sorry, I did 'git commit' without doing 'git add', that is why uint -> uint32_t for this parameter 'I' was not updated here 20 minutes ago. It is fixed now.

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov v-klochkov force-pushed the public_commands_err branch from 69f75f0 to 77a8a52 Compare May 15, 2019 15:13
@bader bader merged commit aa1eb94 into intel:sycl May 16, 2019
@v-klochkov v-klochkov deleted the public_commands_err branch August 6, 2019 14:44
vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Jan 18, 2021
  CONFLICT (content): Merge conflict in clang/lib/CodeGen/CGLoopInfo.cpp
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.

3 participants