[SYCL] Add work_group_num_dim metadata

[jchlanda]

Emit metadata to describe number of dimensions specified in reqd_work_group_size. This is needed in order to be able to use that metadata correctly, since it was specified for OpenCL, and SYCL piggy-backs on it, backends correctly assert if all 3 dimensions are not provided. work_group_num_dim allows the compiler to pad the missing dimensions with 1, while preserving the notion of how many dimensions were specified.

[jchlanda]

Corresponding UR work: oneapi-src/unified-runtime#954

[jchlanda]

Fixes: #9353

[smanna12]

@jchlanda, could you please add FE test?

[frasercrmck]

Regarding the naming, would this problem (and solution) not also apply to work_group_size_hint metadata? If so, could this metadata you're adding be made more general somehow? Maybe just trimming off the reqd_, or something else we can discuss down at the bike shed :)

[jchlanda]

@jchlanda, could you please add FE test?

@smanna12 I've updated the test to use the new metadata node here: 6e43643, do you think this gives us enough coverage?

[jchlanda]

Regarding the naming, would this problem (and solution) not also apply to work_group_size_hint metadata? If so, could this metadata you're adding be made more general somehow? Maybe just trimming off the reqd_, or something else we can discuss down at the bike shed :)

I think you are right, the spec this about the hint:

The number of arguments must match the dimensionality of the work-group used to invoke the kernel, and the order of the arguments matches the order of the dimension extents to the range constructor.

I'm happy to rename it to num_dim_work_group_size, or any other thing that you'd like to suggest @frasercrmck . Naming is hard...

[uditagarwal97]

SYCL changes LGTM 🚀
To prevent accidental merge, I'll approve the PR after the UR PR gets merged and the UR tag gets updated accordingly.

[frasercrmck]

Regarding the naming, would this problem (and solution) not also apply to work_group_size_hint metadata? If so, could this metadata you're adding be made more general somehow? Maybe just trimming off the reqd_, or something else we can discuss down at the bike shed :)

I think you are right, the spec this about the hint:

The number of arguments must match the dimensionality of the work-group used to invoke the kernel, and the order of the arguments matches the order of the dimension extents to the range constructor.

I'm happy to rename it to num_dim_work_group_size, or any other thing that you'd like to suggest @frasercrmck . Naming is hard...

Yeah... work_group_num_dim, num_work_group_dim, num_dim_work_group (in rough priority order)... Not sure the size component is particularly important.

We've perhaps got a short window to rename it once it's merged but it'd be better to get it right first time, before downstream users start relying on the name!

[maksimsab]

sycl-post-link part looks OK to me.

[steffenlarsen]

Aha! Am I understanding this right that the problem is due to the AMD target expecting all 3 dimensions in the "reqd_work_group_size" metadata node? If so, I can see how the current implementation is problematic. The alternative it to change it in upstream, but I suppose the padding may as well be done ASAP and then we can have the "specified dimensions" information separately for the SYCL case.

As for the approach on how to fix it, I think this is reasonable. However, I would prefer if metadata would be the same for all paths. Would it be possible to change the current implementation to always do the padding and then have the dimensions node on the side. It would simplify the general case of checking how many dimensions were specified, and cases that don't care can have a valid 3D value. Note that it might require changes to the SPIR-V translator.

[jchlanda]

Aha! Am I understanding this right that the problem is due to the AMD target expecting all 3 dimensions in the "reqd_work_group_size" metadata node? If so, I can see how the current implementation is problematic. The alternative it to change it in upstream, but I suppose the padding may as well be done ASAP and then we can have the "specified dimensions" information separately for the SYCL case.

That's exactly right, we had a couple of goes at it in the past:

• [AMDGPU] Defaults for missing dimensions in SYCL required wg size llvm/llvm-project#68872
• [AMDGPU] Defaults for missing dimensions in SYCL required wg size llvm/llvm-project#72652

The feedback that we received was that we piggy packed on already existing metadata and we should not modify its semantics, something that I tend to agree with. Preserving the notion of the number of specified dimensions seems like the most elegant solution.

As for the approach on how to fix it, I think this is reasonable. However, I would prefer if metadata would be the same for all paths. Would it be possible to change the current implementation to always do the padding and then have the dimensions node on the side. It would simplify the general case of checking how many dimensions were specified, and cases that don't care can have a valid 3D value. Note that it might require changes to the SPIR-V translator.

I've not worked much with non-GPU targets, so please correct me if I'm wrong here, the reason for padding only for AMD/NVIDIA targets is that those two are the only ones that use the metadata mechanism: emit-program-metadata
. And so my logic was that by not emitting reqd_work_group_size the workaround should not be present either.

[smanna12]

FE changes LGTM.

[kbenzie]

The gen12 failures doesn't seem to be related to this PR.

#13503 needs to merge first to resolve those issues.

[kbenzie]

The gen12 failures doesn't seem to be related to this PR.

#13503 needs to merge first to resolve those issues.

This has now been merged. Pull in the latest sycl branch changes to resolve the ASAN test failures.