This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Improve testing of default header in C++ mode
ClosedPublic

Authored by Anastasia on Mar 18 2019, 5:43 AM.

Details

Summary

Moved testing of the default header into the Driver test and also added a test line for C++ mode in printf test.

Follow up from https://reviews.llvm.org/D59219

Diff Detail

Event Timeline

Anastasia created this revision.Mar 18 2019, 5:43 AM
bader added a comment.Mar 18 2019, 7:04 AM

I see seven OpenCL C tests using -finclude-default-header option and AFAIK, only test/Driver/include-default-header.cl doesn't parse it. Could you also update OpenCL C tests?
I think clang/test/Headers/opencl-c-header.cl and test/Driver/include-default-header.cl fully cover this feature.
All other tests seems to use this option to simplify the test, but with additional cost on parsing this header.

grep -r include-default-header clang/test/

clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -o - -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -o - -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple spir64-unknown-unknown -emit-llvm -o - -cl-std=CL1.2 -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple amdgcn--amdhsa -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple spir64-unknown-unknown -emit-llvm -o - -cl-std=CL1.2 -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple amdgcn--amdhsa -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Driver/include-default-header.cl:// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -emit-llvm -S -### %s
clang/test/Driver/include-default-header.cl:// CHECK-NOT: finclude-default-header
clang/test/Driver/include-default-header.cl:// Make sure we don't pass -finclude-default-header to any commands other than the driver.
clang/test/SemaOpenCL/as_type.cl:// RUN: %clang_cc1 %s -emit-llvm -triple spir-unknown-unknown -finclude-default-header -o - -verify -fsyntax-only
clang/test/SemaOpenCL/printf-format-string-warnings.cl:// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
clang/test/SemaOpenCL/extensions.cl:// Test with -finclude-default-header, which includes opencl-c.h. opencl-c.h
clang/test/SemaOpenCL/extensions.cl:// RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
clang/test/SemaOpenCL/extensions.cl:// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++ -finclude-default-header
clang/test/CodeGenOpenCL/builtins.cl:// RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple spir-unknown-unknown -o - | FileCheck --check-prefix=SZ32 %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple spir64-unknown-unknown -o - | FileCheck --check-prefix=SZ64 --check-prefix=SZ64ONLY %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple amdgcn -o - | FileCheck --check-prefix=SZ64 --check-prefix=AMDGCN %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple amdgcn---opencl -o - | FileCheck --check-prefix=SZ64 --check-prefix=AMDGCN %s

test/Driver/include-default-header.cl
2

According to my understanding test/Driver tests covers only driver component, i.e. all other components like Parser/Sema are not covered by these tests. AFAIK "-###" prints the commands generated by driver, but do not run them.
Do we want to test parsing/Sema for OpenCL C++ mode or do you think that testing OpenCL C mode is enough?

Anastasia updated this revision to Diff 191100.Mar 18 2019, 8:45 AM

Moved testing of default header in C++ mode to test/Headers/opencl-c-header.cl

Anastasia marked an inline comment as done.Mar 18 2019, 8:48 AM
Anastasia added inline comments.
test/Driver/include-default-header.cl
2

I think testing Clang actions in C++ mode with default include is pointless since lang mode doesn't affect anything there. But testing in Sema is indeed what we should do. I have updated the patch now.

However, I am still fixing this test because it wasn't testing anything before.

Thanks for spotting this!

bader accepted this revision.Mar 18 2019, 11:17 AM

OpenCL C++ part looks good. Thanks!

test/Headers/opencl-c-header.cl
57–65 ↗(On Diff #191100)

This test looks strange.
It checks that convert_char_rte is compiled if OpenCL version is not 2.0, but AFAIK it's not deprecated in OpenCL C 2.0, so I don't know why test is written in this way (i.e. ifdef-else-endif).

I think checks should look like this:

char f(char x) {
// Check check OpenCL C 2.0 and OpenCL C++ functionality 
#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)
  ndrange_t t;
  x = ctz(x);
#endif
  return convert_char_rte(x);
}

Probably it's better to fix separately.

This revision is now accepted and ready to land.Mar 18 2019, 11:17 AM
Anastasia marked an inline comment as done.Mar 19 2019, 5:23 AM
Anastasia added inline comments.
test/Headers/opencl-c-header.cl
57–65 ↗(On Diff #191100)

Ok, I think the idea was just to have 2 unique outputs to be checked that we can detect CL2.0 or earlier. But I guess we can do the same with the structure you are suggesting too. I don't mind changing it.

I have to say the header testing is so ad-hoc at the moment and I would like to improve it but because it's so costly we probably need this to be done conditionally. I was thinking to activate it with an extra flag in cmake. Anyway, this is something for the future.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 6:03 AM