[SYCL] Make local accessor argument allocation 1-byte minimum

[steffenlarsen]

Some DPC++ backends, such as OpenCL, does not allow setting local memory kernel arguments with a size of zero bytes. As such, the runtime library can fail with a backend error if a local accessor has a zero-size. These changes make a special case for local accessors with a zero byte size making it allocate a single byte of memory.

[steffenlarsen]

/verify with intel/llvm-test-suite#1001

[cperkinsintel]

code-wise this is fine. But do we really need to special case 0 size? Why isn't failure an option? Are zero sized local accessors a thing?

[steffenlarsen]

code-wise this is fine. But do we really need to special case 0 size? Why isn't failure an option? Are zero sized local accessors a thing?

0-sized local accessors are not disallowed by the SYCL 2020 specification so we should allow it through although it is an edge-case. We cannot simply skip setting the kernel argument as OpenCL will start complaining about unset kernel arguments, so using a minimal-sized allocation seems to be the best solution.

[AerialMantis]

@steffenlarsen I think you're right that the SYCL specification doesn't specify whether zero-sized local accessors, a clarification was added recently which allows zero-sized buffers so there is a precedent to allow it, though perhaps we should raise a ticket for the specification to clarify this?

On a separate note, would I be correct in thinking this change would make #5682 redundant as the backend would never receive a zero-size local memory argument?

[steffenlarsen]

@steffenlarsen I think you're right that the SYCL specification doesn't specify whether zero-sized local accessors, a clarification was added recently which allows zero-sized buffers so there is a precedent to allow it, though perhaps we should raise a ticket for the specification to clarify this?

I am not opposed to having a clarification for it, though I'd argue that since we don't specify an error for it, it should be assumed to work. The minimum allocation here is just an implementation detail and the user should still not access the byte as it is undefined behavior to do so (from the spec's perspective).

On a separate note, would I be correct in thinking this change would make #5682 redundant as the backend would never receive a zero-size local memory argument?

Thank you for pointing that one out! Yes, I would expect so. I assume the CUDA backend is fine without this change, so if you think it would be better off with simply a zero-sized allocation, we can make this strategy backend-dependent, though in that case #5682 would still be an issue.

[AerialMantis]

I am not opposed to having a clarification for it, though I'd argue that since we don't specify an error for it, it should be assumed to work. The minimum allocation here is just an implementation detail and the user should still not access the byte as it is undefined behavior to do so (from the spec's perspective).

I agree, since it's not specified explicitly it's really an implementation detail, and I think this is a fair interpretation.

Thank you for pointing that one out! Yes, I would expect so. I assume the CUDA backend is fine without this change, so if you think it would be better off with simply a zero-sized allocation, we can make this strategy backend-dependent, though in that case #5682 would still be an issue.

That's a good point, the CUDA backend would be fine with this change, we could potentially make this backend-dependent which would allow us to optimize for the zero-size case, though without eliding the argument entirely I suspect this wouldn't have much of an impact, so I'm tempted to say this is better done in the runtime, this also enforces consistent behaviour and avoids errors across all backends.

[keryell]

code-wise this is fine. But do we really need to special case 0 size? Why isn't failure an option?

@cperkinsintel Because this is good quality software and you might want to keep your job. :-) But AMD is hiring... ;-)

[cperkinsintel]

@keryell - it may not be relevant here, but my question about failure being an option is that if a user is making a 0 sized accessor that seems like it is most likely a mistake in their code. By us making it pretend to work, we are hiding their mistake which may lead them to spend a lot of time hunting down some obscure bug in the future. But if it fails the first time they run it, then they'll find their mistake and fix it. That's why I was asking if 0 sized accessors are a real thing. Also, I like my job. ;-)

[keryell]

@cperkinsintel joking aside, when writing some generic code it makes sense to accept a 0-sized accessor instead of asking the programmer to write an exponential number of functions specialized for all the cases.
Of course, it is UB to access to non-existent memory in the case of an accessor of size 0, but in the same way of any out-of-bound access to any accessor.
In another domain, in C++ it is allowed to have a std::array of size 0 https://en.cppreference.com/w/cpp/container/array

[cperkinsintel]

LGTM