[SYCL] Changes to vec/swizzle implementation

[aelovikov-intel]

• Remove expression trees functionality from swizzles following the clarifications made by the SYCL 2020 specification
• Make several previously templated functions non-templated (as they should be according to the specification) as well as aligning other availability constraints with the spec.
• Make named single element (.x()/.r()/etc.) return a swizzle vs previously returned reference to a scalar
• Unify implementation of certain features between sycl::vec and swizzles (named swizzles and multiple binary/unary/op-assign operators)
• Simplified implementation of the vec::(const ArgTys... &args) ctor

Deviations from the specifications added as part of this PR:

• Added non-standard copy-assignment operator for swizzle type. Otherwise, depending on the implementation it might either be implicitly deleted or implicitly defined (potentially with counterintuitive/unintended behavior)
• vec::(const ArgTys... &args) ctor - add an additional NumElements > 1 requirement to resolve the following ambiguity otherwise present with the current is_convertible implementation (a pre-existing deviation from the spec, see below):

  sycl::vec<sycl::half, 1> v{1};  // float/double are ok
  v = 2;     // ambiguous
  v.x() = 3; // ambiguous

Other known remaining differences from the specification inherited from the previous implementation:

• vec::(const ArgTys... &args) ctor requirements have been weaker (std::is_convertible_v<T, DataT>) than required by the specification (must be an exact match of the element_type). Also, unlike the specification, it allows args to be swizzles (in addition to scalars and vecs).
• vec(const vector_t &) ctor is still templated to avoid ambiguity with vec(const DataT &) for one-element vectors
• std::byte support, in particular how some hidden friend operators aren't available and non-standard shift operators are implemented instead, e.g. operator<<(const Self &, int)

Newly added test is mostly copied from @steffenlarsen 's https://github.com/intel/llvm/pull/13026.

[aelovikov-intel]

Not production ready yet, but enough to start non-detailed review.

[aelovikov-intel]

SYCL Nightly (including CTS) passed in https://github.com/intel/llvm/actions/runs/10155132328.

[aelovikov-intel]

We should add tests to validate the working of const and writable swizzles.

I disagree, we should have had (and probably did have) that as part of the PR that added the separation.

Update, that hasn't been merged, I'll shamelessly steal @steffenlarsen 's test...

[uditagarwal97]

The latest changes LGTM.

[aelovikov-intel]

On top of the items highlighted in the PR's description:

• Things like sycl::vec<short, N>{} + 1 fail to compile due to ambiguity, this is aligned with the specification but we might consider changing that

• Logical operators (&&, ||, ...) are under-specified. What is RET for bool, byte or for character type that isn't int8_t nor uint8_t (char/signed char/unsigned char are three distinct types, not two), IIUC

• Any other missing availability requirements for bool/byte?

• The following fails on Windows:

#include <sycl/vector.hpp>
#include <sycl/builtins.hpp>
#include <cassert>
#include <iostream>
#include <iomanip>

int main() {
    sycl::vec<int, 1> v{ 42 };
    auto z = v.x() == 42;

    std::cout << z[0] << std::endl;
    std::cout << std::boolalpha << static_cast<bool>(z) << std::endl;
    std::cout << std::boolalpha << static_cast<bool>(!!z) << std::endl;
    auto a = ((!!(z)) || (std::cout << "my assert" << std::endl , 0));
    static_assert(std::is_same_v<decltype(a), sycl::vec<int, 1>>);
    assert(a[0] == -1);
    assert(z);
    return 0;
}

$ clang++ -fsycl a.cpp  && ./a.exe
-1
true
true
my assert
Assertion failed: z, file a.cpp, line 17

Resolved (?) in the last iteration:

• With the current non-standard availability requirements for vec(const ArgTys& ...args):

  sycl::vec<sycl::half, 1> v{1};  // float/double are ok
  v = 2;     // ambiguous
  v.x() = 3; // ambiguous too

see https://godbolt.org/z/xj7sMf45x. However, aligning to the spec makes the code like ushort4 MaskBits(0xF800 /*r:bits 11-15*/, 0x07E0 /*g:bits 5-10*/, 0x001F /*b:bits 0-4*/, 0x0000) (used in our own image_accessor_util.hpp) illegal. Also note that this particular issue affects CTS: https://github.com/intel/llvm/actions/runs/10253449646/job/28369296785.

On top of that, I think the half case might be even more problematic, see https://github.com/intel/llvm/actions/runs/10223085978/job/28288857472?pr=14789. It seems we have sycl::<half>(/* vector_t */ __fp16) ctor, so something like v = (__fp16)2 would fail even if all the code would 100% match the spec.

[gmlueck]

• std::byte support, in particular how some hidden friend operators aren't available and non-standard shift operators are implemented instead, e.g. operator<<(const Self &, int)

• Logical operators (&&, ||, ...) are under-specified. What is RET for bool, byte or for character type that isn't int8_t nor uint8_t (char/signed char/unsigned char are three distinct types, not two), IIUC

This is a bigger problem that we should try to tackle in a separate PR. The Khronos WG needs to decide whether vec can be used at all with these other types like bool and std::byte. I think vec was originally added to SYCL only for compatibility with OpenCL, which supports only the fixed-width integer types. That's why the descriptions of the member functions are focused on these types.

For now, I'd suggest leaving the existing support for the other types. We can decide later if this should be removed, or if the spec should be clarified for these types. Regarding the RET type for &&, etc., I think it would be reasonable to choose a fixed-width integer return type that matches the size of DataT. For example, when DataT is bool or std::byte, return a vec<int8_t, NumElements>. Is that what we do currently?

[gmlueck]

• Things like sycl::vec<short, N>{} + 1 fail to compile due to ambiguity, this is aligned with the specification but we might consider changing that

• With the current non-standard availability requirements for vec(const ArgTys& ...args):

  sycl::vec<sycl::half, 1> v{1};  // float/double are ok
  v = 2;     // ambiguous
  v.x() = 3; // ambiguous too

Would these ambiguities go away if we added a constraint to:

template <typename... ArgTN> constexpr vec(const ArgTN&... args);

such that this constructor is only available when the number of ArgTn parameter > 1? In the case when the vector has one element, we would rely on this constructor:

explicit constexpr vec(const DataT& arg);

[aelovikov-intel]

For example, when DataT is bool or std::byte, return a vec<int8_t, NumElements>. Is that what we do currently?

For bool or byte's == and != - yes, see https://github.com/intel/llvm/blob/ad3c303feabdbfaf9c56d0ec7560d752808c2dbf/sycl/include/sycl/vector.hpp#L584-L597.

Other logical ops for byte are disabled at https://github.com/intel/llvm/blob/ad3c303feabdbfaf9c56d0ec7560d752808c2dbf/sycl/include/sycl/vector.hpp#L237-L243

[aelovikov-intel]

Would these ambiguities go away if we added a constraint to:

template <typename... ArgTN> constexpr vec(const ArgTN&... args);

such that this constructor is only available when the number of ArgTn parameter > 1?

The change has been pushed.

[aelovikov-intel]

We're still deciding on the needed spec changes, so I've made some temporary changes to be able to use vector.hpp unmodified at godbolt for experiments, see https://godbolt.org/z/asThhfvd3 for example.

[github-actions[bot]]

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.