Skip to content

Conversation

@Pennycook
Copy link
Contributor

Also fixes a bug in the return type of logical_and and logical_or. Closes #646.

The return types of the void specializations of maximum and minimum may require further discussion. Prior to this change, the specification didn't say exactly which operator(s) would be used to determine ordering, and so I tried to honor that in this new wording.

When we introduced the prior wording there was some discussion of trying to align with std::min and std::max (see #285 (comment)). It we want to describe the behavior in terms of equivalent statements, I think the fix is to say:

  • minimum and maximum require T to be LessThanComparable.
  • minimum returns (b < a) ? b : a.
  • maximum returns (a < b) ? b : a.

Also fixes a bug in the return type of logical_and and logical_or.
@Pennycook Pennycook added the bug Something isn't working label Oct 24, 2024
@Pennycook
Copy link
Contributor Author

Pennycook commented Oct 24, 2024

In the WG meeting today we decided that we should define the return types of the minimum and maximum operators. Because the decltype() only describes the return type of the operator, it doesn't actually constrain the implementation of the operator (i.e., it doesn't force an implementation to use a ternary). I'll make this change soon.

Note that the decltype() only describes the return type of the operator, so it
doesn't actually constrain the implementation of the operator.
- Add Precondition that T is Cpp17LessThanComparable.
- Use the same wording for the Returns paragraph.
@Pennycook
Copy link
Contributor Author

This should be read to review, now. Note that the Cpp17LessThanComparable requirement is a precondition and not a constraint. This was surprising to me, but I wanted to follow the ISO C++ definition: https://eel.is/c++draft/alg.min.max

@Pennycook Pennycook added this to the SYCL 2020 milestone Oct 31, 2024
@TApplencourt
Copy link
Contributor

Not in the PR, but if you don't mind to a sneaky update (don't tell our editor :p ) :

float cospi(float x)                (1)
double cospi(double x)              (2)
half cospi(half x)                  (3)

template<typename NonScalar>         (4)
/*return-type*/ cospi(NonScalar x)

This guy (4) is not aligned.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks!

@gmlueck
Copy link
Contributor

gmlueck commented Feb 6, 2025

For my own benefit, this is the Intel tracker to add CTS tests: CMPLRLLVM-65338

aelovikov-intel pushed a commit to intel/llvm that referenced this pull request Apr 22, 2025
fix to return boolean value type of `logical_and`/`logical_or` according
to
KhronosGroup/SYCL-Docs#648

CTS changes will be done in a separate PR

---------

Co-authored-by: Steffen Larsen <[email protected]>
@Pennycook
Copy link
Contributor Author

The change has been merged into DPC++: intel/llvm#17239.

@tomdeakin
Copy link
Contributor

Still waiting CTS I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Waiting for CTS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return types of logical_and and logical_or don't match ISO C++

5 participants