This is an archive of the discontinued LLVM Phabricator instance.

Differential D53200

[OpenCL] Fix serialization of OpenCLExtensionDecls
ClosedPublic

Authored by AlexeySachkov on Oct 12 2018, 9:02 AM.

Details

Reviewers
Anastasia
yaxunl
JDevlieghere
Commits
rG3ccd033c4e4e: Merging r345497:
rG52d674d97499: [OpenCL] Fix serialization of OpenCLExtensionDecls
rL347834: Merging r345497:
rL345497: [OpenCL] Fix serialization of OpenCLExtensionDecls
rC345497: [OpenCL] Fix serialization of OpenCLExtensionDecls
Summary

I recently discovered that adding the following code into opencl-c.h causes
failure of test/Headers/opencl-c-header.cl:

#pragma OPENCL EXTENSION cl_my_ext : begin
void cl_my_ext_foobarbaz();
#pragma OPENCL EXTENSIOn cl_my_ext : end

Clang crashes at the assertion is ASTReader::getGlobalSubmoduleID():

assert(I != M.SubmoduleRemap.end() && "Invalid index into submodule index remap");

The root cause of the problem that to deserialize OPENCL_EXTENSION_DECLS
section ASTReader needs to deserialize a Decl contained in it. In turn,
deserializing a Decl requires information about whether this declaration is
part of a (sub)module, but this information is not read yet because it is
located further in a module file.

Diff Detail

Event Timeline

AlexeySachkov created this revision.Oct 12 2018, 9:02 AM
asavonic added a reviewer: JDevlieghere.Oct 12 2018, 9:12 AM
sidorovd added a subscriber: sidorovd.Oct 15 2018, 8:09 AM
JDevlieghere added a comment.Oct 15 2018, 10:28 AM

Can you add a test case please?

AlexeySachkov updated this revision to Diff 169851.Oct 16 2018, 10:23 AM

Added test

JDevlieghere added a comment.Oct 16 2018, 2:54 PM

The patch looks fine but since I don't know much about opencl I'll leave the LGTM to someone that actually knows this code.

test/Headers/opencl-pragma-extension-begin.h
1

Is this newline intentional?

AlexeySachkov updated this revision to Diff 169978.Oct 17 2018, 3:11 AM

Removed unnecessary empty line from test

AlexeySachkov added a comment.Oct 17 2018, 3:37 AM

@yaxunl could you please review this patch?

It fixes a bug in a feature implemented by you in:

commit c6fb598a301143e9d21156a012cc6ef669ff0188                                                        
Author: Yaxun Liu <Yaxun.Liu@amd.com>                                                                  
Date:   Sun Dec 18 05:18:55 2016 +0000                                                                 
                                                                                                       
    Recommit r289979 [OpenCL] Allow disabling types and declarations associated with extensions        
                                                                                                       
    Fixed undefined behavior due to cast integer to bool in initializer list.                          
                                                                                                       
                                                                                                       
    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@290056 91177308-0d34-0410-b5e6-96231b3b80d8
Anastasia accepted this revision.Oct 19 2018, 10:45 AM
Anastasia added inline comments.
test/Headers/opencl-pragma-extension-begin.cl
2

I think the tests in this folder are for standard includes only but you are testing custom include file here.

Could this be integrated into test/SemaOpenCL/extension-begin.cl? Or if else you could just move to that folder (it might be better to append module to the name in that case).

12

May be it makes sense to test the added extension?

This revision is now accepted and ready to land.Oct 19 2018, 10:45 AM
Anastasia requested changes to this revision.Oct 19 2018, 10:45 AM
This revision now requires changes to proceed.Oct 19 2018, 10:45 AM
AlexeySachkov added inline comments.Oct 20 2018, 12:34 AM
test/Headers/opencl-pragma-extension-begin.cl
2

I think the tests in this folder are for standard includes only but you are testing custom include file here.

Currently, standard include file doesn't contain begin/end ocl pragma directives and I decided to create a special one header for the test.

I will integrate it into existing test for extension-begin pragma.

AlexeySachkov updated this revision to Diff 170601.Oct 23 2018, 4:01 AM

Updated tests

Anastasia accepted this revision.Oct 26 2018, 7:20 AM

LGTM! Thanks!

test/SemaOpenCL/extension-begin.cl
43 ↗(On Diff #170601)

Is this a typo? Should this be 'enabled' instead of 'disabled'?

This revision is now accepted and ready to land.Oct 26 2018, 7:20 AM
AlexeySachkov added inline comments.Oct 26 2018, 9:27 AM
test/SemaOpenCL/extension-begin.cl
43 ↗(On Diff #170601)

I left the diagnostic message the same as it was. Looks like it is a bug in the diagnostic. I can try to fix it, but I would like to do it in a separate patch

Anastasia added inline comments.Oct 26 2018, 11:11 AM
test/SemaOpenCL/extension-begin.cl
43 ↗(On Diff #170601)

Sure! Thanks!

Closed by commit rC345497: [OpenCL] Fix serialization of OpenCLExtensionDecls (authored by asavonic). · Explain WhyOct 29 2018, 4:16 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Serialization/
5 lines
test/
Headers/
4 lines
13 lines

Diff 169851

lib/Serialization/ASTWriter.cpp

Show First 20 Lines • Show All 5,008 Lines • ▼ Show 20 LinesASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
WriteHeaderSearch(PP.getHeaderSearchInfo()); WriteHeaderSearch(PP.getHeaderSearchInfo());
WriteSelectors(SemaRef); WriteSelectors(SemaRef);
WriteReferencedSelectorsPool(SemaRef); WriteReferencedSelectorsPool(SemaRef);
WriteLateParsedTemplates(SemaRef); WriteLateParsedTemplates(SemaRef);
WriteIdentifierTable(PP, SemaRef.IdResolver, isModule); WriteIdentifierTable(PP, SemaRef.IdResolver, isModule);
WriteFPPragmaOptions(SemaRef.getFPOptions()); WriteFPPragmaOptions(SemaRef.getFPOptions());
WriteOpenCLExtensions(SemaRef); WriteOpenCLExtensions(SemaRef);
WriteOpenCLExtensionTypes(SemaRef); WriteOpenCLExtensionTypes(SemaRef);
WriteOpenCLExtensionDecls(SemaRef);
WriteCUDAPragmas(SemaRef); WriteCUDAPragmas(SemaRef);
// If we're emitting a module, write out the submodule information. // If we're emitting a module, write out the submodule information.
if (WritingModule) if (WritingModule)
WriteSubmodules(WritingModule); WriteSubmodules(WritingModule);
// We need to have information about submodules to correctly deserialize
// decls from OpenCLExtensionDecls block
WriteOpenCLExtensionDecls(SemaRef);
Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes); Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes);
// Write the record containing external, unnamed definitions. // Write the record containing external, unnamed definitions.
if (!EagerlyDeserializedDecls.empty()) if (!EagerlyDeserializedDecls.empty())
Stream.EmitRecord(EAGERLY_DESERIALIZED_DECLS, EagerlyDeserializedDecls); Stream.EmitRecord(EAGERLY_DESERIALIZED_DECLS, EagerlyDeserializedDecls);
if (!ModularCodegenDecls.empty()) if (!ModularCodegenDecls.empty())
Stream.EmitRecord(MODULAR_CODEGEN_DECLS, ModularCodegenDecls); Stream.EmitRecord(MODULAR_CODEGEN_DECLS, ModularCodegenDecls);
▲ Show 20 Lines • Show All 1,908 Lines • Show Last 20 Lines

test/Headers/opencl-pragma-extension-begin.h

  • This file was added.
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

Is this newline intentional?

JDevlieghere: Is this newline intentional?
#pragma OPENCL EXTENSION cl_my_ext : begin
void cl_my_ext_foo();
#pragma OPENCL EXTENSION cl_my_ext : end

test/Headers/opencl-pragma-extension-begin.cl

  • This file was added.
// RUN: rm -rf %t.ocl.pragma.ext.begin
// RUN: mkdir -p %t.ocl.pragma.ext.begin
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

I think the tests in this folder are for standard includes only but you are testing custom include file here.

Could this be integrated into test/SemaOpenCL/extension-begin.cl? Or if else you could just move to that folder (it might be better to append module to the name in that case).

Anastasia: I think the tests in this folder are for standard includes only but you are testing custom…
AlexeySachkovAuthorUnsubmitted
Not Done
ReplyInline Actions

I think the tests in this folder are for standard includes only but you are testing custom include file here.

Currently, standard include file doesn't contain begin/end ocl pragma directives and I decided to create a special one header for the test.

I will integrate it into existing test for extension-begin pragma.

AlexeySachkov: > I think the tests in this folder are for standard includes only but you are testing custom…
//
// RUN: %clang_cc1 -cl-std=CL1.2 -include %S/opencl-pragma-extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.ocl.pragma.ext.begin %s 2>&1
//
// RUN: rm -rf %t.ocl.pragma.ext.begin
// RUN: mkdir -p %t.ocl.pragma.ext.begin
//
// RUN: %clang_cc1 -cl-std=CL2.0 -include %S/opencl-pragma-extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.ocl.pragma.ext.begin %s 2>&1
void __kernel test(__global int *data) {
*data = 10;
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

May be it makes sense to test the added extension?

Anastasia: May be it makes sense to test the added extension?
}