reqd_work_group_size and max_work_group_size argument checking

[smanna12]

// Checks correctness of mutual usage of different work_group_size attributes:
reqd_work_group_size, max_work_group_size and max_global_work_dim.

1. Values of reqd_work_group_size arguments shall be equal or less than values coming from max_work_group_size.
2. In case the value of 'max_global_work_dim' attribute equals to 0 we shall ensure that if max_work_group_size and
   reqd_work_group_size attributes exist, they hold equal values (1, 1, 1).
3. Values of reqd_work_group_size arguments shall be equal or greater than values coming from reqd_work_group_size.

The order of the arguments to reqd_work_group_size in SYCL and OpenCL are reversed. In the OpenCL syntax, reqd_work_group_size(X,Y,Z), X is the index that increments the fastest, followed by Y, then Z. If we preserve that definition of X, Y, and Z, then in SYCL the attribute is specified as reqd_work_group_size(Z,Y,X).

This is same for max_work_group_size attribute also.

Question:

1. don't we have to care about the difference between the OpenCL spellings and the SYCL spelling when deciding whether to
   look at X or Z?

I didn't see any documentation whether we should consider all arguments: X, Y, Z of reqd_work_group_size and max_work_group_size attributes here to check correctness of mutual usage of different work_group_size attributes.
So it's not clear what the diagnostic behavior should be.

[smanna12]

Tgging @MrSidims , @GarveyJoe , @AaronBallman for the discussion about this bug. Thanks

[AaronBallman]

3. Values of reqd_work_group_size arguments shall be equal or greater than values coming from reqd_work_group_size.

I'm not following along on this one. :-)

Question:
don't we have to care about the difference between the OpenCL spellings and the SYCL spelling when deciding whether to
look at X or Z?

I think we do for reqd_work_group_size since there's an OpenCL spelling and SYCL reversed the argument order. I don't think we do for max_work_group_size because there's no OpenCL spelling supported for that (but it does raise the really sad question of "did we specify the order for max_work_group_size to be Z, Y, X like we did for reqd_work_group_size?" because I can't help but notice the arguments are in X, Y, Z order in Attr.td and we don't document the arguments at all in Attr.td).

[smanna12]

I don't think we do for max_work_group_size because there's no OpenCL spelling supported for that (but it does raise the really sad question of "did we specify the order for max_work_group_size to be Z, Y, X like we did for reqd_work_group_size?

This seems to be a bug with existing implementation since we are currently swapping the argument values during codegen part for max_work_group_size attribute.

We should not reverse the argument order since there is no OpenCL spelling supported for max_work_group_size with DPC++.

// For a SYCLDevice SYCLIntelMaxWorkGroupSizeAttr arguments are reversed.
if (getLangOpts().SYCLIsDevice)
std::swap(XDimVal, ZDimVal);

[MrSidims]

Initially, all of the work group attributes had X, Y, Z order (same order as in OpenCL and as in SYCL runtime/language work group specifiers). Later, SYCL runtime/language work group specifiers were swapped from left-most order to the right-most (following C++ guidance). And the same was done for the attributes. And we see it created a bit of mess.

don't we have to care about the difference between the OpenCL spellings and the SYCL spelling when deciding whether to
look at X or Z?

I think we don't. From semantic-checking perspective, if the attributes are applied to a kernel in SYCL code: we should consider, that both attributes are having SYCL arguments order rule. From code gen perspective what matter is if user's code is correctly lowered to LLVM IR, so backend could correctly recognize it and do its job (for that swap is an OK solution).

I think, that the most transparent way would be to split OpenCL attributes from SYCL attributes, but I'm not really a fun of code duplication.

P.S. while looking in AttrDocs.td I found another description inaccurancy:
2421 If the intel::reqd_work_group_size or cl::reqd_work_group_size
2422 attribute is specified on a declaration along with a
2423 intel::num_simd_work_items attribute, the work group size attribute
2424 argument (the first argument) must be evenly divisible by the argument specified
2425 in the intel::num_simd_work_items attribute.

It shouldn't be "the first argument". Yet again, an artifact of engineering docs not following language evolution.

[MrSidims]

BTW, thanks for bringing this, I think clearing this is very helpful for maintaining the code!

[AaronBallman]

don't we have to care about the difference between the OpenCL spellings and the SYCL spelling when deciding whether to
look at X or Z?

I think we don't. From semantic-checking perspective, if the attributes are applied to a kernel in SYCL code: we should consider, that both attributes are having SYCL arguments order rule.

Are SYCL and OpenCL modes mutually exclusive? The situation I am most concerned by is with `attribute((reqd_work_group_size)`` being used in SYCL code when it is an OpenCL attribute, and the resulting argument order confusion.

I think, that the most transparent way would be to split OpenCL attributes from SYCL attributes, but I'm not really a fun of code duplication.

One possibility that may or may not be a terrible idea would be to have OpenCLReqdWorkGroupAttr and SYCLReqdWorkGroupAttr as semantic attributes in Attr.td, but give them a common base class ReqdWorkGroupAttr with a helper function like:

let AdditionalMembers = [{
  Optional<llvm::APSInt> getWorkGroupSize() const {
    if (const auto *OpenCL = dyn_cast<OpenCLReqdWorkGroupAttr>(this))
      return OpenCL->getXDim();
    if (const auto *SYCL = dyn_cast<SYCLReqdWorkGroupAttr>(this))
      return SYCL->getZDim();
    llvm_unreachable("Unknown attribute class");  
  }
}];

This would allow us to use ReqdWorkGroupAttr the same way we always have been, give us a handy place to hang the "I don't care what argument it is, but get me the work group size" API, and gives us a way to specify the attribute argument names differently for OpenCL and SYCL. We don't have any attributes that do this sort of thing currently, so there's not a good attribute on which to model these changes (IOW, "there may be surprises.")

[smanna12]

P.S. while looking in AttrDocs.td I found another description inaccurancy:
2421 If the intel::reqd_work_group_size or cl::reqd_work_group_size
2422 attribute is specified on a declaration along with a
2423 intel::num_simd_work_items attribute, the work group size attribute
2424 argument (the first argument) must be evenly divisible by the argument specified
2425 in the intel::num_simd_work_items attribute.

It shouldn't be "the first argument". Yet again, an artifact of engineering docs not following language evolution.

Thanks @MrSidims for looking into this bug.

Yes, it should not be the "first argument" here, It should be last one. I misunderstood.
I am working on this PR #3337 to fix the issue.

[MrSidims]

Are SYCL and OpenCL modes mutually exclusive?

That is a very good point. AFAIU OpenCL operability mode implies not only to host code, but also to device code (see: 4.8.9.4 Defining kernels using OpenCL C kernel objects). So the modes aren't exclusive.

One possibility that may or may not be a terrible idea would be

TBH I kinda like it.

[bader]

Are SYCL and OpenCL modes mutually exclusive?

That is a very good point. AFAIU OpenCL operability mode implies not only to host code, but also to device code (see: 4.8.9.4 Defining kernels using OpenCL C kernel objects). So the modes aren't exclusive.

What does it mean? Are you saying that we should be able to compile a source file with both modes enabled (i.e. -fsycl-device-only -x cl)?

[MrSidims]

Are you saying that we should be able to compile a source file with both modes enabled (i.e. -fsycl-device-only -x cl)?

Not really. The spec doesn't specifies what compiler (or in which mode) OpenCL C code should be compiled. But in our implementation I see an online_compiler extension, which allows to compile OpenCL C sources nearby SYCL device code. At least that is how I got it, please correct me if I'm wrong.

[MrSidims]

Okay, I just tried clang++ -fsycl -fsycl-device-only online_compiler_OpenCL.cpp and I see there is no device code generated.

[AaronBallman]

The reason I ask is because __attribute__((reqd_work_group_size)) is an attribute specified by the OpenCL standard (https://www.khronos.org/registry/OpenCL/sdk/1.0/docs/man/xhtml/functionQualifiers.html) and so I don't think we can treat it as having the SYCL argument ordering when compiling for SYCL.

[bader]

I think @MrSidims misinterpret the question. Modes are mutually exclusive, so we can't compile the same kernel attribute in OpenCL and SYCL modes, but I agree that even if document the argument order difference, it might lead to non-obvious bugs.