This is an archive of the discontinued LLVM Phabricator instance.

Differential D105988

[OpenCL] NULL introduced prior to v2.0
ClosedPublic

Authored by Topotuna on Jul 14 2021, 8:15 AM.

Details

Reviewers
Anastasia
azabaznov
yaxunl
Commits
rG5c63bf3abdc7: [OpenCL] Add NULL to standards prior to v2.0.
Summary

NULL macro maps to ((void*)0) for OpenCL versions earlier than 2.0

NULL was undefined in OpenCL prior to version 2.0. However, the
language specification states that `macro names defined by the C99
specification but not currently supported by OpenCL are reserved
for future use`. Therefore, application developers cannot redefine
NULL.

The change is supposed to resolve inconsistency between language
versions. Currently there is no apparent reason why NULL should
be kept undefined.

Fixes llvm.org/PR48102

Diff Detail

Event Timeline

Topotuna created this revision.Jul 14 2021, 8:15 AM
Herald added subscribers: ldrumm, Anastasia, yaxunl. · View Herald TranscriptJul 14 2021, 8:15 AM
Topotuna requested review of this revision.Jul 14 2021, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2021, 8:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Anastasia added reviewers: Anastasia, azabaznov, yaxunl.Jul 14 2021, 8:17 AM
Anastasia added a comment.Jul 14 2021, 8:19 AM

This seems reasonable.

clang/test/SemaOpenCL/null_literal.cl
1–4

I think we should remove this NULL definition but instead add -finclude-default-header -fdeclare-opencl-builtins to test the definition you are adding in the header.

Topotuna added a child revision: D105987: [C++4OpenCL] NULL redefined as nullptr.Jul 14 2021, 8:24 AM
Harbormaster completed remote builds in B113987: Diff 358612.Jul 14 2021, 8:52 AM
Topotuna updated this revision to Diff 358625.Jul 14 2021, 8:57 AM

Added compile time flags

-finclude-default-header -fdeclare-opencl-builtins

instead of

#define NULL ((void*)0)
Harbormaster completed remote builds in B113998: Diff 358625.Jul 14 2021, 8:58 AM
Topotuna updated this revision to Diff 358628.Jul 14 2021, 9:02 AM

Wrong diff file added previously

Topotuna marked an inline comment as done.Jul 14 2021, 9:06 AM
Harbormaster completed remote builds in B114000: Diff 358628.Jul 14 2021, 10:10 AM
Anastasia added inline comments.Jul 15 2021, 3:37 AM
clang/lib/Headers/opencl-c-base.h
167

I think we should drop #if here since the condition now includes both OpenCL kernel languages that clang supports.

clang/test/SemaOpenCL/null_literal.cl
1–4

Ok, is this a full diff? https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Because the file appears to be incompletely displayed or something...

Topotuna updated this revision to Diff 358911.Jul 15 2021, 3:51 AM

Full diff now added. Redundant condition #if removed

Topotuna marked 2 inline comments as done.Jul 15 2021, 3:53 AM
Topotuna added inline comments.
clang/test/SemaOpenCL/null_literal.cl
1–4

Apologies. That was not a full diff. Should be fixed now.

Harbormaster completed remote builds in B114195: Diff 358911.Jul 15 2021, 4:27 AM
Anastasia accepted this revision.Jul 15 2021, 3:31 PM

LGTM! Thanks

This revision is now accepted and ready to land.Jul 15 2021, 3:31 PM
Topotuna mentioned this in D105987: [C++4OpenCL] NULL redefined as nullptr.Jul 16 2021, 2:23 AM
Topotuna edited child revisions, added: D106254: [OpenCL][NFC] Refactors conditional versioning; removed: D105987: [C++4OpenCL] NULL redefined as nullptr.Jul 20 2021, 1:01 AM
This revision was landed with ongoing or failed builds.Jul 23 2021, 3:55 AM
Closed by commit rG5c63bf3abdc7: [OpenCL] Add NULL to standards prior to v2.0. (authored by Anastasia). · Explain Why
This revision was automatically updated to reflect the committed changes.
Anastasia added a commit: rG5c63bf3abdc7: [OpenCL] Add NULL to standards prior to v2.0..

Revision Contents

PathSize
clang/
lib/
Headers/
2 lines
test/
SemaOpenCL/
8 lines

Diff 361152

clang/lib/Headers/opencl-c-base.h

Show First 20 Lines • Show All 158 Lines • ▼ Show 20 Lines
typedef half half16 __attribute__((ext_vector_type(16))); typedef half half16 __attribute__((ext_vector_type(16)));
#endif #endif
#ifdef cl_khr_fp64 #ifdef cl_khr_fp64
#if __OPENCL_C_VERSION__ < CL_VERSION_1_2 #if __OPENCL_C_VERSION__ < CL_VERSION_1_2
#pragma OPENCL EXTENSION cl_khr_fp64 : enable #pragma OPENCL EXTENSION cl_khr_fp64 : enable
#endif #endif
typedef double double2 __attribute__((ext_vector_type(2))); typedef double double2 __attribute__((ext_vector_type(2)));
typedef double double3 __attribute__((ext_vector_type(3))); typedef double double3 __attribute__((ext_vector_type(3)));
typedef double double4 __attribute__((ext_vector_type(4))); typedef double double4 __attribute__((ext_vector_type(4)));
AnastasiaUnsubmitted
Done
ReplyInline Actions

I think we should drop #if here since the condition now includes both OpenCL kernel languages that clang supports.

Anastasia: I think we should drop `#if` here since the condition now includes both OpenCL kernel languages…
typedef double double8 __attribute__((ext_vector_type(8))); typedef double double8 __attribute__((ext_vector_type(8)));
typedef double double16 __attribute__((ext_vector_type(16))); typedef double double16 __attribute__((ext_vector_type(16)));
#endif #endif
#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
#define NULL ((void*)0) #define NULL ((void*)0)
#endif
/** /**
* Value of maximum non-infinite single-precision floating-point * Value of maximum non-infinite single-precision floating-point
* number. * number.
*/ */
#define MAXFLOAT 0x1.fffffep127f #define MAXFLOAT 0x1.fffffep127f
/** /**
▲ Show 20 Lines • Show All 544 Lines • Show Last 20 Lines

clang/test/SemaOpenCL/null_literal.cl

// RUN: %clang_cc1 -verify %s // RUN: %clang_cc1 -cl-std=CL1.0 -fdeclare-opencl-builtins -finclude-default-header -verify %s
// RUN: %clang_cc1 -cl-std=CL2.0 -verify %s // RUN: %clang_cc1 -cl-std=CL1.1 -fdeclare-opencl-builtins -finclude-default-header -verify %s
// RUN: %clang_cc1 -cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header -verify %s
#define NULL ((void*)0) // RUN: %clang_cc1 -cl-std=CL2.0 -fdeclare-opencl-builtins -finclude-default-header -verify %s
AnastasiaUnsubmitted
Done
ReplyInline Actions

I think we should remove this NULL definition but instead add -finclude-default-header -fdeclare-opencl-builtins to test the definition you are adding in the header.

Anastasia: I think we should remove this `NULL` definition but instead add `-finclude-default-header…
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

Ok, is this a full diff? https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Because the file appears to be incompletely displayed or something...

Anastasia: Ok, is this a full diff? https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web…
TopotunaAuthorUnsubmitted
Done
ReplyInline Actions

Apologies. That was not a full diff. Should be fixed now.

Topotuna: Apologies. That was not a full diff. Should be fixed now.
void foo(){ void foo(){
global int* ptr1 = NULL; global int* ptr1 = NULL;
global int* ptr2 = (global void*)0; global int* ptr2 = (global void*)0;
constant int* ptr3 = NULL; constant int* ptr3 = NULL;
Show All 17 Lines