-
Notifications
You must be signed in to change notification settings - Fork 733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL] Change vec operators to be friends #12396
[SYCL] Change vec operators to be friends #12396
Conversation
This commit changes operators for sycl::vec to be defined like they are in the SYCL specification, i.e. friend functions instead of members. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
// RUN: %{run} %t.out | ||
|
||
// This test currently fails on AMD HIP due to an unresolved memcmp function. | ||
// XFAIL: hip_amd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@npmiller - It seems that this test fails on AMD HIP due to memcmp
. I suspect it might be the vector comparison operators, but I have not reproduced it locally. Once this is merged I will open an issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a stdlib memcmp? We haven't added libcxx support for AMD yet so that would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error was:
# .---command stderr------------
# | lld: error: undefined hidden symbol: memcmp
# | >>> referenced by lto.tmp:(typeinfo name for main::'lambda163'(sycl::_V1::handler&)::operator()(sycl::_V1::handler&) const::'lambda'())
# | >>> referenced by lto.tmp:(typeinfo name for main::'lambda163'(sycl::_V1::handler&)::operator()(sycl::_V1::handler&) const::'lambda'())
# | >>> referenced by lto.tmp:(typeinfo name for main::'lambda263'(sycl::_V1::handler&)::operator()(sycl::_V1::handler&) const::'lambda'())
# | >>> referenced 1 more times
# | llvm-foreach:
# | clang++: error: amdgcn-link command failed with exit code 1 (use -v to see invocation)
# `-----------------------------
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds like it, it's fine to keep it as XFAIL
Friendly ping @intel/llvm-reviewers-runtime @cperkinsintel |
1 similar comment
Friendly ping @intel/llvm-reviewers-runtime @cperkinsintel |
sycl/include/sycl/types.hpp
Outdated
} | ||
#endif // defined(__INTEL_PREVIEW_BREAKING_CHANGES) | ||
|
||
#if !defined(__INTEL_PREVIEW_BREAKING_CHANGES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are trying to avoid using #else
for long blocks. It makes the closing comment easier to understand.
For example, you've changed this so now it ends like so:
#endif // defined(__INTEL_PREVIEW_BREAKING_CHANGES)
Which would lead the casual reader to think ('oh, this block above is for the preview") which is wrong. The block immediately above is the ELSE clause of the preview.
I know it seems pedantic, but especially with this preview stuff where we have long blocks of semi-repeating code, let's try to avoid #else
in long blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I disagree with that, but it is not a hill I want to die on. We could simply change the comment on the #endif
to make it clearer and the #else
, I would argue, makes the association clearer between the two blocks. Either way, I've split them up.
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
This commit does a partial revert of intel#12396 as it is unclear from the specification how these operators should behave for swizzles. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
This commit changes operators for sycl::vec to be defined like they are in the SYCL specification, i.e. friend functions instead of members.