[OpenCL] Include opencl-c.h by default as a clang module

[yaxunl]

Summary

Include opencl-c.h by default as a module to utilize the automatic AST caching mechanism of clang modules.

Add an option -finclude-default-header to enable default header for OpenCL, which is off by default.

[yaxunl]

Fix regressions in unit test due to ConstructJob disables builtin include path.

[Anastasia]

include/clang/Basic/LangOptions.def

219

I was just thinking whether it would make sense to do the opposite i.e. to have a flag that would enable loading of the header (which won't be loaded by default). This might be better for compatibility.

The issues might arise because the users already have their own BIF declaration mechanisms in place in Clang or because of the parsing speed increase. The latter can affect testing significantly. Does the module mechanism use PCH?

Have you compared the speed with and without including the OpenCL header? Is there any difference?

[yaxunl]

Revised as Anastasia suggested.

Add option -finclude-default-header to include OpenCL default header file, which is off by default.
Add test to make sure the AST of the header is really cached and works for different OpenCL versions.

[yaxunl]

include/clang/Basic/LangOptions.def

219

I agree and has made the change.

The module caches AST of the header file, which is essentially the same thing as PCH.

I checked a typical OpenCL progam. Without including header,

---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
0.0160 (100.0%)   0.0080 (100.0%)   0.0240 (100.0%)   0.0211 (100.0%)  Clang front-end timer
0.0160 (100.0%)   0.0080 (100.0%)   0.0240 (100.0%)   0.0211 (100.0%)  Total

With header but without module caching,

---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
0.2160 (100.0%)   0.0120 (100.0%)   0.2280 (100.0%)   0.2271 (100.0%)  Clang front-end timer
0.2160 (100.0%)   0.0120 (100.0%)   0.2280 (100.0%)   0.2271 (100.0%)  Total

With header and module caching,

---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
0.0160 (100.0%)   0.0080 (100.0%)   0.0240 (100.0%)   0.0222 ( 93.6%)  Clang front-end timer
0.0000 (  0.0%)   0.0000 (  0.0%)   0.0000 (  0.0%)   0.0008 (  3.5%)  Reading modules
0.0000 (  0.0%)   0.0000 (  0.0%)   0.0000 (  0.0%)   0.0007 (  2.9%)  Loading /home/yaxunl/llvm-tot/llvm/tools/clang/test/CodeGenOpenCL/./JJHF9CF78G8Z/opencl_c-3RXFI66A6CUM7.pcm
0.0160 (100.0%)   0.0080 (100.0%)   0.0240 (100.0%)   0.0237 (100.0%)  Total

So the caching is very effective and essentially makes the header loading time to be ignored.

[pxli168]

LGTM!
Thanks!

[Anastasia]

lib/Frontend/CompilerInvocation.cpp

1782

Is this change being tested somewhere?

[bader]

test/Headers/opencl-c-header.cl

30–31

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.

[yaxunl]

Add a test for -fblocks as Anastasia suggested.

[yaxunl]

test/Headers/opencl-c-header.cl

30–31

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

[Anastasia]

test/Frontend/opencl-blocks.cl

11 ↗

(On Diff #59398)

This is not part of this change, but I was just wondering whether it would make sense to update the message to refer to OpenCL version that allows blocks (similarly to other cases).

We might need to create different diagnostic or use select option?

[yaxunl]

Improve diag msg for fblocks.

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

7347 ↗

(On Diff #59431)

-> for OpenCL version 2.0 or above

test/Headers/opencl-c-header.cl

27

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?

30–31

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!

47

This line seems exactly the same as line 67.

[yaxunl]

Update the diagnositic msg for fblocks.

test/Headers/opencl-c-header.cl

27

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.

30–31

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

47

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.

[Anastasia]

test/Headers/opencl-c-header.cl

27

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

30–31

Sounds cool. Thanks!

47

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

[yaxunl]

test/Headers/opencl-c-header.cl

27

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?

47

No. It should use the cached module.

[Anastasia]

test/Headers/opencl-c-header.cl

27

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.

47

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

[yaxunl]

Modified the test for module as Anastasia suggested.

[yaxunl]

test/Headers/opencl-c-header.cl

27

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.

47

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.

[Anastasia]

test/Headers/opencl-c-header.cl

47

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?

[yaxunl]

test/Headers/opencl-c-header.cl

47

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]

test/Headers/opencl-c-header.cl

30–31

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.

[Anastasia]

test/Headers/opencl-c-header.cl

30–31

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...

47

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.

[yaxunl]

Update the test as Anastasia suggested.

[Anastasia]

LGTM, I think it would be nice if the block related code goes in a separate commit.

Thanks!

[yaxunl]

In D20444#457588, @Anastasia wrote:

LGTM, I think it would be nice if the block related code goes in a separate commit.

Thanks!

Will do. Thanks.

include/clang/Basic/LangOptions.def

 LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field type alignment")
 LANGOPT(HexagonQdsp6Compat , 1, 0, "hexagon-qdsp6 backward compatibility")
 LANGOPT(ObjCAutoRefCount , 1, 0, "Objective-C automated reference counting")
 LANGOPT(ObjCWeakRuntime     , 1, 0, "__weak support in the ARC runtime")
 LANGOPT(ObjCWeak            , 1, 0, "Objective-C __weak in ARC and MRC files")
 LANGOPT(ObjCSubscriptingLegacyRuntime         , 1, 0, "Subscripting support in legacy ObjectiveC runtime")
 LANGOPT(FakeAddressSpaceMap , 1, 0, "OpenCL fake address space map")
 ENUM_LANGOPT(AddressSpaceMapMangling , AddrSpaceMapMangling, 2, ASMM_Target, "OpenCL address space map mangling mode")
-
+LANGOPT(NoDefaultHeader, 1, 0, "Do not include default header file for OpenCL")
 BENIGN_LANGOPT(DelayedTemplateParsing , 1, 0, "delayed template parsing")
 LANGOPT(BlocksRuntimeOptional , 1, 0, "optional blocks runtime")
 
 ENUM_LANGOPT(GC, GCMode, 2, NonGC, "Objective-C Garbage Collection mode")
 ENUM_LANGOPT(ValueVisibilityMode, Visibility, 3, DefaultVisibility,
              "value symbol visibility")
 ENUM_LANGOPT(TypeVisibilityMode, Visibility, 3, DefaultVisibility,
              "type symbol visibility")

include/clang/Driver/CC1Options.td

 def fnative_half_type: Flag<["-"], "fnative-half-type">,
   HelpText<"Use the native half type for __fp16 instead of promoting to float">;
 def fnative_half_arguments_and_returns : Flag<["-"], "fnative-half-arguments-and-returns">,
   HelpText<"Use the native __fp16 type for arguments and returns (and skip ABI-specific lowering)">;
 def fallow_half_arguments_and_returns : Flag<["-"], "fallow-half-arguments-and-returns">,
   HelpText<"Allow function arguments and returns of type half">;
 def fdefault_calling_conv_EQ : Joined<["-"], "fdefault-calling-conv=">,
   HelpText<"Set default MS calling convention">;
+def fno_default_header : Flag<["-"], "fno-default-header">,
+  HelpText<"Do not include the default header file for OpenCL">;
 
 // C++ TSes.
 def fcoroutines : Flag<["-"], "fcoroutines">,
   HelpText<"Enable support for the C++ Coroutines TS">;
 
 //===----------------------------------------------------------------------===//
 // Header Search Options
 //===----------------------------------------------------------------------===//

include/clang/Frontend/CompilerInvocation.h

public:
   static std::string GetResourcesPath(const char *Argv0, void *MainAddr);
 
   /// \brief Set language defaults for the given input language and
   /// language standard in the given LangOptions object.
   ///
   /// \param Opts - The LangOptions object to set up.
   /// \param IK - The input language.
   /// \param T - The target triple.
+  /// \param PPOpts - The PreprocessorOptions affected.
   /// \param LangStd - The input language standard.
   static void setLangDefaults(LangOptions &Opts, InputKind IK,
-                   const llvm::Triple &T,
+                   const llvm::Triple &T, PreprocessorOptions &PPOpts,
                    LangStandard::Kind LangStd = LangStandard::lang_unspecified);
   
   /// \brief Retrieve a module hash string that is suitable for uniquely 
   /// identifying the conditions under which the module was built.
   std::string getModuleHash() const;
   
   /// @}
   /// @name Option Subgroups

lib/Frontend/CompilerInvocation.cpp

         A->getValue(), A->getOption().matches(OPT_system_header_prefix));
 
   for (const Arg *A : Args.filtered(OPT_ivfsoverlay))
     Opts.AddVFSOverlayFile(A->getValue());
 }
 
 void CompilerInvocation::setLangDefaults(LangOptions &Opts, InputKind IK,
                                          const llvm::Triple &T,
+                                         PreprocessorOptions &PPOpts,
                                          LangStandard::Kind LangStd) {
   // Set some properties which depend solely on the input kind; it would be nice
   // to move these to the language standard, and have the driver resolve the
   // input kind + language standard.
   if (IK == IK_Asm) {
     Opts.AsmPreprocessor = 1;
   } else if (IK == IK_ObjC ||
              IK == IK_ObjCXX ||

   if (Opts.OpenCL) {
     Opts.AltiVec = 0;
     Opts.ZVector = 0;
     Opts.CXXOperatorNames = 1;
     Opts.LaxVectorConversions = 0;
     Opts.DefaultFPContract = 1;
     Opts.NativeHalfType = 1;
     Opts.NativeHalfArgsAndReturns = 1;
+    // Include default header file for OpenCL.
+    if (!Opts.NoDefaultHeader) {
+      Opts.Modules = true;
+      Opts.ImplicitModules = true;
+      PPOpts.Includes.push_back("opencl-c.h");
+    }
   }
 
   Opts.CUDA = IK == IK_CUDA || IK == IK_PreprocessedCuda ||
               LangStd == LangStandard::lang_cuda;
 
   // OpenCL and C++ both have bool, true, false keywords.
   Opts.Bool = Opts.OpenCL || Opts.CPlusPlus;
 

 
   diags.Report(diag::err_drv_invalid_value)
     << arg->getAsString(args) << value;
   return DefaultVisibility;
 }
 
 static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK,
                           const TargetOptions &TargetOpts,
+                          PreprocessorOptions &PPOpts,
                           DiagnosticsEngine &Diags) {
   // FIXME: Cleanup per-file based stuff.
   LangStandard::Kind LangStd = LangStandard::lang_unspecified;
   if (const Arg *A = Args.getLastArg(OPT_std_EQ)) {
     LangStd = llvm::StringSwitch<LangStandard::Kind>(A->getValue())
 #define LANGSTANDARD(id, name, desc, features) \
       .Case(name, LangStandard::lang_##id)
 #include "clang/Frontend/LangStandards.def"

     if (OpenCLLangStd == LangStandard::lang_unspecified) {
       Diags.Report(diag::err_drv_invalid_value)
       << A->getAsString(Args) << A->getValue();
     }
     else
       LangStd = OpenCLLangStd;
   }
 
+  Opts.NoDefaultHeader = Args.hasArg(OPT_fno_default_header);
+
   llvm::Triple T(TargetOpts.Triple);
-  CompilerInvocation::setLangDefaults(Opts, IK, T, LangStd);
+  CompilerInvocation::setLangDefaults(Opts, IK, T, PPOpts, LangStd);
 
   // We abuse '-f[no-]gnu-keywords' to force overriding all GNU-extension
   // keywords. This behavior is provided by GCC's poorly named '-fasm' flag,
   // while a subset (the non-C++ GNU keywords) is provided by GCC's
   // '-fgnu-keywords'. Clang conflates the two for simplicity under the single
   // name, as it doesn't seem a useful distinction.
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
                                   Opts.GNUKeywords);

   Opts.ObjCExceptions = Args.hasArg(OPT_fobjc_exceptions);
   Opts.CXXExceptions = Args.hasArg(OPT_fcxx_exceptions);
   Opts.SjLjExceptions = Args.hasArg(OPT_fsjlj_exceptions);
   Opts.ExternCNoUnwind = Args.hasArg(OPT_fexternc_nounwind);
   Opts.TraditionalCPP = Args.hasArg(OPT_traditional_cpp);
 
   Opts.RTTI = Opts.CPlusPlus && !Args.hasArg(OPT_fno_rtti);
   Opts.RTTIData = Opts.RTTI && !Args.hasArg(OPT_fno_rtti_data);
-  Opts.Blocks = Args.hasArg(OPT_fblocks);
+  Opts.Blocks = Args.hasArg(OPT_fblocks) || (Opts.OpenCL
+    && Opts.OpenCLVersion >= 200 && !Args.hasArg(OPT_fno_blocks));
   Opts.BlocksRuntimeOptional = Args.hasArg(OPT_fblocks_runtime_optional);
   Opts.Coroutines = Args.hasArg(OPT_fcoroutines);
   Opts.Modules = Args.hasArg(OPT_fmodules);
   Opts.ModulesStrictDeclUse = Args.hasArg(OPT_fmodules_strict_decluse);
   Opts.ModulesDeclUse =
       Args.hasArg(OPT_fmodules_decluse) || Opts.ModulesStrictDeclUse;
   Opts.ModulesLocalVisibility =
       Args.hasArg(OPT_fmodules_local_submodule_visibility);
   Opts.ModulesSearchAll = Opts.Modules &&
     !Args.hasArg(OPT_fno_modules_search_all) &&
     Args.hasArg(OPT_fmodules_search_all);
   Opts.ModulesErrorRecovery = !Args.hasArg(OPT_fno_modules_error_recovery);
   Opts.ImplicitModules = !Args.hasArg(OPT_fno_implicit_modules);

     // PIClevel and PIELevel are needed during code generation and this should be
     // set regardless of the input type.
     LangOpts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
     LangOpts.PIELevel = getLastArgIntValue(Args, OPT_pie_level, 0, Diags);
     parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
                         Diags, LangOpts.Sanitize);
   } else {
     // Other LangOpts are only initialzed when the input is not AST or LLVM IR.
-    ParseLangArgs(LangOpts, Args, DashX, Res.getTargetOpts(), Diags);
+    ParseLangArgs(LangOpts, Args, DashX, Res.getTargetOpts(),
+      Res.getPreprocessorOpts(), Diags);
     if (Res.getFrontendOpts().ProgramAction == frontend::RewriteObjC)
       LangOpts.ObjCExceptions = 1;
   }
 
   // During CUDA device-side compilation, the aux triple is the triple used for
   // host compilation.
   if (LangOpts.CUDA && LangOpts.CUDAIsDevice) {
     Res.getTargetOpts().HostTriple = Res.getFrontendOpts().AuxTriple;

lib/Headers/module.modulemap

explicit module htm {
       header "htmxlintrin.h"
     }
 
     explicit module zvector {
       requires zvector, vx
       header "vecintrin.h"
     }
   }
+  
+  explicit module opencl_c {
+    requires opencl
+    header "opencl-c.h"
+  }
 }
 
 module _Builtin_stddef_max_align_t [system] [extern_c] {
   header "__stddef_max_align_t.h"
 }

test/CodeGenOpenCL/bool_cast.cl

 // RUN: %clang_cc1 %s -emit-llvm -o - -O0 | FileCheck %s
 
-typedef unsigned char uchar4 __attribute((ext_vector_type(4)));
-typedef unsigned int int4 __attribute((ext_vector_type(4)));
-typedef float float4 __attribute((ext_vector_type(4)));
-
 // CHECK-LABEL: define void @ker()
 void kernel ker() {
   bool t = true;
   int4 vec4 = (int4)t;
 // CHECK: {{%.*}} = load i8, i8* %t, align 1
 // CHECK: {{%.*}} = trunc i8 {{%.*}} to i1
 // CHECK: {{%.*}} = sext i1 {{%.*}} to i32
 // CHECK: {{%.*}} = insertelement <4 x i32> undef, i32 {{%.*}}, i32 0

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 -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 -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 -emit-llvm -o - %s | 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 -fno-default-header -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 -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
 char f(char x) {
   return convert_char_rte(x);
+#ifdef NO_HEADER
+  //expected-warning@-2{{implicit declaration of function 'convert_char_rte' is invalid in C99}}
+#endif
 }

test/Parser/opencl-atomics-cl20.cl

-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
+// ToDo: a bug causing cached AST of header containing typedef of atomic_int for CL1.0. After the bug is fixed
+// -fno-default-header should be removed.
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -fno-default-header
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20 -DEXT
 
 #ifdef EXT
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics:enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics:enable
 #pragma OPENCL EXTENSION cl_khr_fp64:enable
 #if __OPENCL_C_VERSION__ >= CL_VERSION_1_2

unittests/AST/MatchVerifier.h

case Lang_CXX:
     Args.push_back("-std=c++98");
     FileName = "input.cc";
     break;
   case Lang_CXX11:
     Args.push_back("-std=c++11");
     FileName = "input.cc";
     break;
   case Lang_OpenCL:
+    // ConstructJob disables builtin include path which is required by OpenCL
+    // default header file, therefore default header needs to be disabled.
+    Args.push_back("-Xclang");
+    Args.push_back("-fno-default-header");
     FileName = "input.cl";
     break;
   case Lang_OBJCXX:
     FileName = "input.mm";
     break;
   }
 
   // Default to failure in case callback is never called