Page MenuHomePhabricator

[OpenCL] Fix printing of types with signed prefix in arg info metadata
ClosedPublic

Authored by Anastasia on Feb 5 2021, 10:34 AM.

Details

Summary

OpenCL spec doesn't seem to describe the behavior correctly for type printing in GetKernelArgInfo (https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#clGetKernelArgInfo):

The type name returned will be the argument type name as it was declared with any whitespace removed. If argument type name is an unsigned scalar type (i.e. unsigned char, unsigned short, unsigned int, unsigned long), uchar, ushort, uint and ulong will be returned.

The example with unsigned doesn't seem to comply with the general statement in the first sentence. It is believed that it actually meant to say that the single word spelling should be used for types i.e.

unsigned type -> utype
signed type -> type

This patch fixed the printing of type with signed prefix in spelling.

Diff Detail

Event Timeline

Anastasia created this revision.Feb 5 2021, 10:34 AM
Anastasia requested review of this revision.Feb 5 2021, 10:34 AM
stuart requested changes to this revision.Feb 5 2021, 11:34 AM
stuart added a subscriber: stuart.

Looks good. Small nit about the test case.

clang/lib/CodeGen/CodeGenModule.cpp
1498–1499

It'd make sense to push this comment down one line, above the consume_front("unsigned ") call, as it doesn't apply to the consume_front("signed ") call - or reword it so it covers both substitutions.

clang/test/CodeGenOpenCL/kernel-arg-info.cl
110

signed char would be a better test case, here (although it may be good to test signed int as well).

I believe (but I haven't checked in detail) that for int the canonical naming is either int or unsigned int (i.e. signed int will not occur) whereas for char, the canonical naming is unsigned char, signed char (explicitly stated) or simply char (unstated signedness). (I am basing this on metadata that I have seen, and the understanding that in C, the signedness of char with no explicit signed or unsigned specifier is implementation-defined.)

This revision now requires changes to proceed.Feb 5 2021, 11:34 AM
Anastasia updated this revision to Diff 322087.Feb 8 2021, 5:33 AM

Use char instead of int in the test.

Anastasia marked an inline comment as done.Feb 8 2021, 8:11 AM
Anastasia added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
1498–1499

This has been addressed in the parent review:
https://reviews.llvm.org/D96151

stuart requested changes to this revision.Feb 9 2021, 3:07 AM

Looks good, just some suggestions about the test.

clang/test/CodeGenOpenCL/kernel-arg-info.cl
110

Suggest sc1 and sc2 as these are now signed chars, not signed ints.

112–118

Would suggest SCHAR_AS_QUAL and SCHAR_TY now that this is using a signed char.

159–162

... and here.

This revision now requires changes to proceed.Feb 9 2021, 3:07 AM
Anastasia updated this revision to Diff 322330.Feb 9 2021, 3:34 AM

Renamed variables in CHECK statements

Anastasia added inline comments.Feb 9 2021, 3:35 AM
clang/test/CodeGenOpenCL/kernel-arg-info.cl
112–118

FYI also SCHAR_ARG_NAMES and SCHAR_QUAL

Anastasia marked an inline comment as done.Feb 9 2021, 3:36 AM
Anastasia marked an inline comment as done.
mantognini accepted this revision.Feb 9 2021, 6:33 AM

LGTM, thanks. All the comments seem to be addressed.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 9 2021, 7:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 7:13 AM