[SYCL][NVPTX] Emit reqd_work_group_size attributes as NVVM annotations

[frasercrmck]

Only emit the provided values as annotations in the LLVM IR. The NVPTX backend will pad missing values with 1s. This suits the fact that the attribute must provide as many values as the dimensionality of the work-group, and we can assume that the work-group size of unused dimensions is 1.

[elizabethandrews]

The code looks ok to me but this attribute is handled differently for different backends now and I do not know if that is ok (#13600). @premanandrao @steffenlarsen please weigh in here.

[frasercrmck]

The code looks ok to me but this attribute is handled differently for different backends now and I do not know if that is ok (#13600). @premanandrao @steffenlarsen please weigh in here.

It's not ideal, no. I do think NVPTX.cpp is a good place to add NVPTX-specific metadata, and CodeGenFunction.cpp is a good place to add target-independent metadata.

We do need this NVPTX-specific lowering as the NVVM annotations are the only way the NVPTX backend is going to make use of the attributes - it's just ignored otherwise. I think the most regretful outcome if that we have the same information repeated in multiple different ways: see also how we are already handling [[intel::max_work_group_size]] attribute this way. For example, intel::max_work_group_size(4, 8, 16) gives us:

define  void @foo() max_work_group_size !28 {
   ...
}

!nvvm.annotations = !{19, !20, !21}

!19 = !{ptr @_ZTSZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_E1C, !"maxntidx", i32 16}
!20 = !{ptr @_ZTSZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_E1C, !"maxntidy", i32 8}
!21 = !{ptr @_ZTSZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_E1C, !"maxntidz", i32 4}

!28 = !{i32 16, i32 8, i32 4}

Perhaps one way of making it nicer would be to lower from Attribute -> Function Metadata in CodeGenFunction.cpp, then lower the Function Metadata to NVVM annotations in NVPTX.cpp. That may give a clearer lowering path, separating out responsibilities.

[frasercrmck]

Sorry, accidentally deleted the branch

[steffenlarsen]

Great! I attempted to do this a long time ago (#3755), but there were problems due to us padding the dimensions of the attribute at the time. Lovely to see it finally happening. 😄

I am not too concerned about it taking a new path for NVPTX and generating special NVVM metadata. From what I remember, the current behavior for the attribute is heavily based on the SPIR-V translator consuming the metadata that the OpenCL-related attribute set a precedence for.

An alternative could be to have a late-stage LLVM transformation pass for consuming the "reqd_work_group_size" metadata and generating these NVVM attributes. A benefit of this is that we could use this for handling "sycl-work-group-size" produced by the SYCL compile-time property alternative for this attribute.

[premanandrao]

The code looks ok to me but this attribute is handled differently for different backends now and I do not know if that is ok (#13600). @premanandrao @steffenlarsen please weigh in here.

It's not ideal, no. I do think NVPTX.cpp is a good place to add NVPTX-specific metadata, and CodeGenFunction.cpp is a good place to add target-independent metadata.

We do need this NVPTX-specific lowering as the NVVM annotations are the only way the NVPTX backend is going to make use of the attributes - it's just ignored otherwise. I think the most regretful outcome if that we have the same information repeated in multiple different ways: see also how we are already handling [[intel::max_work_group_size]] attribute this way. For example, intel::max_work_group_size(4, 8, 16) gives us:

define  void @foo() max_work_group_size !28 {
   ...
}

!nvvm.annotations = !{19, !20, !21}

!19 = !{ptr @_ZTSZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_E1C, !"maxntidx", i32 16}
!20 = !{ptr @_ZTSZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_E1C, !"maxntidy", i32 8}
!21 = !{ptr @_ZTSZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_E1C, !"maxntidz", i32 4}

!28 = !{i32 16, i32 8, i32 4}

Perhaps one way of making it nicer would be to lower from Attribute -> Function Metadata in CodeGenFunction.cpp, then lower the Function Metadata to NVVM annotations in NVPTX.cpp. That may give a clearer lowering path, separating out responsibilities.

Thanks for the explanation. I don't have a strong opinion on changing what you have, so I will approve this. But @elizabethandrews might, but she is out until at least Monday.

[frasercrmck]

Great! I attempted to do this a long time ago (#3755), but there were problems due to us padding the dimensions of the attribute at the time. Lovely to see it finally happening. 😄

I am not too concerned about it taking a new path for NVPTX and generating special NVVM metadata. From what I remember, the current behavior for the attribute is heavily based on the SPIR-V translator consuming the metadata that the OpenCL-related attribute set a precedence for.

An alternative could be to have a late-stage LLVM transformation pass for consuming the "reqd_work_group_size" metadata and generating these NVVM attributes. A benefit of this is that we could use this for handling "sycl-work-group-size" produced by the SYCL compile-time property alternative for this attribute.

No problem! I wasn't aware you'd already tried it, sorry.

Yeah I agree the current lowering for NVPTX (and I'm guessing AMDGPU) leaves something to be desired. I think I'll look into this in a subsequent patch, if that's alright?

[steffenlarsen]

No problem! I wasn't aware you'd already tried it, sorry.

Don't be sorry! It was just to say that I think this is the right direction and it's great to see it actually happening. 😄

Yeah I agree the current lowering for NVPTX (and I'm guessing AMDGPU) leaves something to be desired. I think I'll look into this in a subsequent patch, if that's alright?

You could, but I figure it will involve moving the transformation done here to a later stage to handle both the property and the attribute the same. I don't see a problem in that though, but I'll let @elizabethandrews and @premanandrao have the final say.

[elizabethandrews]

Sorry for the delay. I have been battling covid and working reduced hours. This PR LGTM.

My uneasiness has more to do with these attributes in general and not your implementation.

[smanna12]

LGTM. Thanks

[frasercrmck]

This PR looks ready to merge, @intel/llvm-gatekeepers - thanks!