Skip to content
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

Intended behavior of operator* in id<1> #711

Open
Pennycook opened this issue Feb 4, 2025 · 5 comments · May be fixed by #726
Open

Intended behavior of operator* in id<1> #711

Pennycook opened this issue Feb 4, 2025 · 5 comments · May be fixed by #726

Comments

@Pennycook
Copy link
Contributor

What is the intended behavior of the following code?

#include <sycl/sycl.hpp>

int foo(sycl::id<1> i, int j)
{
    return i * j;
}

It compiles with DPC++, but fails with SimSYCL (celerity/SimSYCL#40) due to an ambiguous overload.

I wrote a toy implementation of id based on what I think the specification says (https://godbolt.org/z/P8MTWW1s7) and the operator overload works as I'd expect.

@fknorr
Copy link

fknorr commented Feb 4, 2025

When you add the argument-swapped overload (the spec lists both), the call becomes ambiguous: https://godbolt.org/z/74MYofMhY.

Also see the DPC++ workaround for this situation.

@Pennycook
Copy link
Contributor Author

When you add the argument-swapped overload (the spec lists both), the call becomes ambiguous: https://godbolt.org/z/74MYofMhY.

Oh, you're right! I missed that the second overload was defined, because of the way the specification groups them in the synopsis:

  // OP is: +, -, *, /, %, <<, >>, &, |, ^, &&, ||, <, >, <=, >=
  friend id operatorOP(const id& lhs, const id& rhs) { /* ... */
  }
  friend id operatorOP(const id& lhs, const size_t& rhs) { /* ... */
  }

  // OP is: +=, -=, *=, /=, %=, <<=, >>=, &=, |=, ^=
  friend id& operatorOP(id& lhs, const id& rhs) { /* ... */
  }
  friend id& operatorOP(id& lhs, const size_t& rhs) { /* ... */
  }

  // OP is: +, -, *, /, %, <<, >>, &, |, ^, &&, ||, <, >, <=, >=
  friend id operatorOP(const size_t& lhs, const id& rhs) { /* ... */
  }

Re-ordering those for clarity might be a good idea.

Also see the DPC++ workaround for this situation.

I want to make sure I'm reading this right. The workaround here is that DPC++ defines all of these operators not just for size_t, but for all integral types?

@fknorr
Copy link

fknorr commented Feb 4, 2025

Also see the DPC++ workaround for this situation.

I want to make sure I'm reading this right. The workaround here is that DPC++ defines all of these operators not just for size_t, but for all integral types?

That's how I interpret the code. It appears these templated conversion operators were first introduced here: intel/llvm#939

@Pennycook
Copy link
Contributor Author

I've just discovered this is a duplicate of #539.

@keryell
Copy link
Member

keryell commented Feb 6, 2025

I've just discovered this is a duplicate of #539.

We have a huge issue backlog unfortunately. 🤒

@Pennycook Pennycook linked a pull request Feb 14, 2025 that will close this issue
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 a pull request may close this issue.

3 participants