Page MenuHomePhabricator
Differential D20444 Diff 57837 test/Headers/opencl-c-header.cl

Changeset View

Standalone View

View Options

test/Headers/opencl-c-header.cl

// RUN: %clang_cc1 -internal-isystem ../../lib/Headers -include opencl-c.h -emit-llvm -o - %s | FileCheck %s // RUN: %clang_cc1 -fno-default-header -internal-isystem ../../lib/Headers -include opencl-c.h -emit-llvm -o - %s | FileCheck %s
// RUN: %clang_cc1 -internal-isystem ../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -cl-std=CL1.1| FileCheck %s // RUN: %clang_cc1 -fno-default-header -internal-isystem ../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -cl-std=CL1.1| FileCheck %s
// RUN: %clang_cc1 -internal-isystem ../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -cl-std=CL1.2| FileCheck %s // RUN: %clang_cc1 -fno-default-header -internal-isystem ../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -cl-std=CL1.2| FileCheck %s
// RUN: %clang_cc1 -internal-isystem ../../lib/Headers -include opencl-c.h -fblocks -emit-llvm -o - %s -cl-std=CL2.0| FileCheck %s // RUN: %clang_cc1 -fno-default-header -internal-isystem ../../lib/Headers -include opencl-c.h -fblocks -emit-llvm -o - %s -cl-std=CL2.0| FileCheck %s
// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
// RUN: %clang_cc1 -emit-llvm -verify -fno-default-header -DNO_HEADER %s
// CHECK: _Z16convert_char_rtec // CHECK: _Z16convert_char_rtec
char f(char x) { char f(char x) {
return convert_char_rte(x); return convert_char_rte(x);
#ifdef NO_HEADER
//expected-warning@-2{{implicit declaration of function 'convert_char_rte' is invalid in C99}}
#endif
} }
baderUnsubmitted
Not Done
ReplyInline Actions

It would be nice to re-use that part of module that agnostic to OpenCL version. Most of the OpenCL 2.0 declarations are already in the module for OpenCL 1.0.
Pointer/size_t arguments could another reason to re-parse the header.

Just FYI: at the moment we use chained PCHes to reduce the footprint of the PCHes we ship with OpenCL implementation.
The chain looks like: <common part> + <standard specific part> + <platform specific part>, where 60% is occupied by <common part>, which is shared across all possible compilations.

bader: It would be nice to re-use that part of module that agnostic to OpenCL version. Most of the…
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

We can discuss that as the next step for improving the header.

yaxunl: We can discuss that as the next step for improving the header.
AnastasiaUnsubmitted
Done
ReplyInline Actions

Indeed PCH size is an issue. I don't quite get why it expands from the original source code size by a factor or 2. Something to look at!

Anastasia: Indeed PCH size is an issue. I don't quite get why it expands from the original source code…
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

I can take a look why the PCH size is much larger than the header file itself.

yaxunl: I can take a look why the PCH size is much larger than the header file itself.
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

Sounds cool. Thanks!

Anastasia: Sounds cool. Thanks!
yaxunlAuthorUnsubmitted
Not Done
ReplyInline Actions

I checked the size of the pch file. The largest chunk is for function declarations. Since it contains about 30k declarations, each one is about 50 bytes on average, so total size is about 1.6MB.

The AST of function decl is written by ASTDeclWriter::VisitFunctionDecl http://clang.llvm.org/doxygen/ASTWriterDecl_8cpp_source.html

A code snippet is as follows:

515 Record.push_back((int)D->SClass); // FIXME: stable encoding

516   Record.push_back(D->IsInline);
517   Record.push_back(D->IsInlineSpecified);
518   Record.push_back(D->IsVirtualAsWritten);
519   Record.push_back(D->IsPure);
520   Record.push_back(D->HasInheritedPrototype);
521   Record.push_back(D->HasWrittenPrototype);
522   Record.push_back(D->IsDeleted);
523   Record.push_back(D->IsTrivial);
524   Record.push_back(D->IsDefaulted);
525   Record.push_back(D->IsExplicitlyDefaulted);
526   Record.push_back(D->HasImplicitReturnZero);
527   Record.push_back(D->IsConstexpr);
528   Record.push_back(D->HasSkippedBody);
529   Record.push_back(D->IsLateTemplateParsed);
530   Record.push_back(D->getLinkageInternal());
531   Record.AddSourceLocation(D->getLocEnd());

Record is like a buffer which will be written to file. It uses a vector of int64 to store the values, so it takes space.

yaxunl: I checked the size of the pch file. The largest chunk is for function declarations. Since it…
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

It feels the problem is that it has to record all the attributes even though most of them are missing. Quite wasteful! Wondering if variable length solution would be possible...

Anastasia: It feels the problem is that it has to record all the attributes even though most of them are…
AnastasiaUnsubmitted
Done
ReplyInline Actions

Sam, what was the aim of this test?

I was just wondering if we should instead check that the file is exactly the same i.e. hasn't been overwritten with exactly the same content?

Anastasia: Sam, what was the aim of this test? I was just wondering if we should instead check that the…
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

This checks whether the content has changed, which is the same approach used by test/Modules/fmodules-validate-once-per-build-session.c to make sure a module is not re-compiled.

It seems there is no good way otherwise. stat -c %Y filename can get the modification time but it is in seconds, which is not accurate enough.

yaxunl: This checks whether the content has changed, which is the same approach used by…
AnastasiaUnsubmitted
Done
ReplyInline Actions

I see, but is diffing accurate here? Because if the file is regenerated but with exactly the same content it won't be caught...

Anastasia: I see, but is diffing accurate here? Because if the file is regenerated but with exactly the…
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

It cannot detect if the file was re-written with the same content.

There is one way we can do that:
get the modified time of the file
sleep 1 second
get the modified time of the file again and compare

but it will slow down the test by 1 second. do we really want to do that?

yaxunl: It cannot detect if the file was re-written with the same content. There is one way we can do…
AnastasiaUnsubmitted
Done
ReplyInline Actions

Not desirable to increase the testing time. I was wondering if we could amend the attribute of the file let's say run chmod on it? I guess if it's regenerated it would get default attributes again?

Otherwise, I would rather skip testing uniqueness, if we can't do it properly. We rely on the existing modules functionality anyways which is already being tested elsewhere.

Anastasia: Not desirable to increase the testing time. I was wondering if we could amend the attribute of…
yaxunlAuthorUnsubmitted
Not Done
ReplyInline Actions

I changed the module to read only. If Clang tries to create a new module, it will fail. Also add a check to make sure module is read.

yaxunl: I changed the module to read only. If Clang tries to create a new module, it will fail. Also…
AnastasiaUnsubmitted
Done
ReplyInline Actions

This line seems exactly the same as line 67.

Anastasia: This line seems exactly the same as line 67.
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

Right. The first compilation generates the module and the second compilation uses the cached module. The module is compiled for different OpenCL version and triple to make sure the cached module still works.

yaxunl: Right. The first compilation generates the module and the second compilation uses the cached…
AnastasiaUnsubmitted
Done
ReplyInline Actions

So in this line it will be regenerated because the line above used different triple?

Anastasia: So in this line it will be regenerated because the line above used different triple?
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

No. It should use the cached module.

yaxunl: No. It should use the cached module.
AnastasiaUnsubmitted
Done
ReplyInline Actions

Ok, but it doesn't seem like there is something different being tested to line 67.

Anastasia: Ok, but it doesn't seem like there is something different being tested to line 67.
yaxunlAuthorUnsubmitted
Not Done
ReplyInline Actions

I added check to the second compilation to make sure module is read, also changed the modules to be read only so that they won't be created again.

yaxunl: I added check to the second compilation to make sure module is read, also changed the modules…
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

Ok, now I see what you are testing here. :)

Do you think we could add:

CHECK-NOT: Reading modules

For the cases the modules are regenerated new?

Anastasia: Ok, now I see what you are testing here. :) Do you think we could add: CHECK-NOT: Reading…
yaxunlAuthorUnsubmitted
Not Done
ReplyInline Actions

In the case the modules are generated as new, Clang will generate the module first and then load it. So in the time report, you still see 'Reading modules'.

yaxunl: In the case the modules are generated as new, Clang will generate the module first and then…
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

In this case, should you be testing it in regenerated mode too?

Also would it make sense to add:

CHECK-NOT: _Z3ctzc

to make sure there isn't OpenCL 2.0 functionality.

Anastasia: In this case, should you be testing it in regenerated mode too? Also would it make sense to…