This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Mark kernel arguments as ABI aligned
ClosedPublic

Authored by nikic on Feb 3 2022, 5:50 AM.

Details

Summary

Following the discussion on D118229, this marks all pointer-typed kernel arguments as having ABI alignment, hopefully addressing the regression from that change.

Diff Detail

Event Timeline

nikic created this revision.Feb 3 2022, 5:50 AM
nikic requested review of this revision.Feb 3 2022, 5:50 AM
yaxunl added a comment.Feb 3 2022, 6:28 AM

LGTM. I will defer to Anastasia.

clang/lib/CodeGen/CGCall.cpp
2478

Pls add a reference to the OpenCL spec in the comment:

OpenCL spec v3.0.10 Appendix C Alignment of Application Data Types:
The user is responsible for ensuring that pointers passed into and out of OpenCL kernels are natively aligned relative to the data type of the parameter as defined in the kernel language and SPIR-V specifications.

arsenm added inline comments.Feb 3 2022, 6:48 AM
clang/lib/CodeGen/CGCall.cpp
2479

Why does opencl need special treatment here? Isn't this the default behavior for all abi/languages?

nikic updated this revision to Diff 405625.Feb 3 2022, 6:52 AM

Add spec quote.

nikic marked an inline comment as done.Feb 3 2022, 6:53 AM
nikic added inline comments.
clang/lib/CodeGen/CGCall.cpp
2479

In C++, references are required to be aligned (see the case directly above this), but pointers generally aren't.

arsenm added inline comments.Feb 3 2022, 6:53 AM
clang/lib/CodeGen/CGCall.cpp
2479

Plus I would assume this also applies to non-kernels

2479

But pre-opaque pointers, these would have ended up using the datalayout ABI alignment?

nikic added inline comments.Feb 3 2022, 6:58 AM
clang/lib/CodeGen/CGCall.cpp
2479

No, at least not in generic code. We used to assume this, but it has been fixed a few releases back.

Anastasia added inline comments.Feb 3 2022, 8:29 AM
clang/lib/CodeGen/CGCall.cpp
2479

I agree it doesn't seem to be kernel-specific, should this apply to all functions? However, I wonder why this should need a special case in OpenCL C compared to C behavior?

I feel the spec reference is not entirely related as all it's trying to say is that the host code is responsible to make sure the data are aligned in accordance with the device.

Looking at LangRef it doesn't state what the default values for alignments are:
https://llvm.org/docs/LangRef.html#parameter-attributes
but the change does seem to make sense in principle.

Anastasia added inline comments.Feb 3 2022, 8:35 AM
clang/lib/CodeGen/CGCall.cpp
2479

Ok I think this might be a more relevant reference:
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#alignment-of-types

For arguments to a __kernel function declared to be a pointer to a data type, the OpenCL compiler can assume that the pointee is always appropriately aligned as required by the data type.

However it still feels like it says that we expect data passed from host are adequately aligned?

So I feel like this might be an implementation specific aspect that spec doesn't actually regulate...

arsenm added inline comments.Feb 3 2022, 8:36 AM
clang/lib/CodeGen/CGCall.cpp
2479

The conformance test does check the low bits of the pointer are aligned

yaxunl added a subscriber: rjmccall.Feb 3 2022, 8:36 AM
yaxunl added inline comments.
clang/lib/CodeGen/CGCall.cpp
2479

I don't think in general we can assume pointers are naturally aligned. There are valid use cases of pointers to non-aligned types, e.g. a member of a packed structure. A function with a pointer-type argument should accept pointer arguments to non-aligned types. OpenCL pointer-type kernel arguments are special cases where the alignment is guaranteed by the spec.

@rjmccall

Anastasia added inline comments.Feb 3 2022, 12:47 PM
clang/lib/CodeGen/CGCall.cpp
2479

There are valid use cases of pointers to non-aligned types, e.g. a member of a packed structure.

What if such types are used in kernel arguments, I can't see any related restrictions...

The conformance test does check the low bits of the pointer are aligned

Ok, just to understand a bit more... has the failure been introduced recently or is this a new test?

arsenm added inline comments.Feb 3 2022, 12:48 PM
clang/lib/CodeGen/CGCall.cpp
2479

The test started failing recently due to no longer using the pointee element types' ABI alignment. clang needs to start providing it in the IR explicitly

nikic added inline comments.Feb 3 2022, 12:54 PM
clang/lib/CodeGen/CGCall.cpp
2479

There are valid use cases of pointers to non-aligned types, e.g. a member of a packed structure.

What if such types are used in kernel arguments, I can't see any related restrictions...

Isn't this covered by the part of the OpenCL spec you quoted above? Pointers can generally be unaligned, but for pointer arguments to kernels the spec requires that they are aligned.

Anastasia added inline comments.Feb 3 2022, 1:07 PM
clang/lib/CodeGen/CGCall.cpp
2479

I am still confused why the fix is only needed for the kernel functions now... Has there been any change in LLVM IR that triggers this issue? Is there some specific handling happening in for the functions that are regular and therefore are being called in contrast to kernels that are normally not called on the kernel side?

Anastasia added inline comments.Feb 3 2022, 1:11 PM
clang/test/CodeGenOpenCL/amdgpu-call-kernel.cl
3

Let's choose the pointee type with width different from pointer size... also should we test types that @yaxunl mentioned e.g with packed structs or void?

Anastasia added inline comments.Feb 3 2022, 3:38 PM
clang/lib/CodeGen/CGCall.cpp
2479

To unconfuse (mainly myself) the difference here is that OpenCL spec actually fixes the alignment of types in contrast to C/C++ that only specifies the alignment requirements. However OpenCL spec does special case kernel function parameters which is why the normal functions still remain regulated by C/C++ rules... not sure it was intentional but the change is reasonable.

nikic updated this revision to Diff 405938.Feb 4 2022, 6:01 AM

Update spec reference, add a dedicated alignment test.

arsenm added inline comments.Feb 4 2022, 7:24 AM
clang/test/CodeGenOpenCL/kernel-param-alignment.cl
11–14

Should throw in some vectors for good measure

nikic updated this revision to Diff 405991.Feb 4 2022, 9:06 AM

Check vector alignment.

arsenm accepted this revision.Feb 8 2022, 7:07 AM
This revision is now accepted and ready to land.Feb 8 2022, 7:07 AM
This revision was landed with ongoing or failed builds.Feb 8 2022, 7:17 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2022, 7:17 AM