[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Elad Cohen via cfe-commits]

eladcohen added a comment.

In https://reviews.llvm.org/D25337#571877, @rsmith wrote:

> I really don't like the command-line interface you're proposing here. It 
> seems like it will be extremely confusing what something like `-fmodules 
> -fexclusive-builtin-modules` means, for instance (usually, later `-f` flags 
> override earlier ones, so does this *turn off* modules for everything other 
> than builtin headers?). Instead, we should use a command-line interface that 
> more naturally / obviously composes with other flags.
>
> It seems like the behavior you want here is precisely:
>
>   `-fmodules -fno-implicit-module-maps -fmodule-map-file= dir>/include/module.modulemap`
>   
>
> (The `-fmodules-validate-system-headers` part would only make sense for 
> people using clang directly from their build area; installed / released 
> versions of clang should not need this. Adding it makes your flag compose 
> poorly with other uses of modules.)
>
> I'd be happy with you adding a flag that is equivalent to 
> `-fmodule-map-file=/include/module.modulemap`.


You are right, this is the behavior I was aiming at. I just hoped I could get a 
single flag to achieve it instead of 3, but I agree that it doesn't behave well 
with the other module flags.
I uploaded the new flag in a separate review: https://reviews.llvm.org/D25767

> Also, it seems unlikely to me that we would ever turn this on by default. It 
> will cause havoc for distributed builds, especially ones that run compilation 
> actions in 'clean' environments, as (for instance) it will cause the complete 
> set of intrinsics headers to be compiled into a module on every compile. So 
> if this is your proposed solution for slow compiles using intrinsics headers, 
> you should be aware that this is not a complete solution to that problem.

What do you mean by 'clean' environments? As in no intermediate artifacts (such 
as the .pcm files) could be cached?

Do you have any thoughts or suggestions on how we can make the solution more 
complete?

I guess one possibility to handle this matter (IIRC mentioned by Chandler in 
one of the discussions) is to pre-compile the actual builtins module as part of 
the build system. But IIUC this is not feasible since there could be a huge 
matrix of all the different configurations we will have to maintain, right?


https://reviews.llvm.org/D25337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Elad Cohen via cfe-commits]

eladcohen added a comment.

In https://reviews.llvm.org/D25337#571845, @bruno wrote:

> > The long answer is that there is a history of problems regarding the 
> > intrinsic files:
> >  http://lists.llvm.org/pipermail/cfe-dev/2016-May/048837.html
> >  http://lists.llvm.org/pipermail/cfe-dev/2016-September/050943.html
> >  Mainly compatibility issues because MSVC makes all the intrinsics 
> > available all the time, while in clang this is not always the case (On 
> > Windows the different h files are not-included unless asked for explicitly 
> > since AVX512 was added which impacted compile-time).
>
> Thinking a bit more about this: did you ever consider a solution where 
> "immintrin.h" (or any similar other) would use "#include_next" and 
> "__has_include_next" to point to a second level (user specified) immintrin.h, 
> which then could use ifdefs to only include what's relevant?


I don't think we really want the users to have to maintain an updated user 
specific version of immintrin.h. Moreover, I think that at the end we actually 
do want clang to always include all the intrinsics same as MSVC.

>> A suggestion was made to try and mitigate this by reducing the compile time 
>> using modules (only for the x86 intrinsics). This patch aims at adding this 
>> option. The new compile flag is just a milestone - if all goes well, we can 
>> turn this on by default (Then we won't actually need to use a specific 
>> compile flag, and we could always include all the intrinsic h files without 
>> their long compile time).
> 
> The speedup sounds nice, but it seems awkward IMO to have such specific 
> behavior for x86 intrinsics only.

The feature itself is not x86 specific. It can be used for all the clang 
builtins. As for enabling it by default, I did have only x86 in mind but it can 
be done for all targets as well. (will probably require checking that all the 
other intrinsic headers are modular and that the modulemap is up to date).


https://reviews.llvm.org/D25337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Richard Smith via cfe-commits]

rsmith added a comment.

I really don't like the command-line interface you're proposing here. It seems 
like it will be extremely confusing what something like `-fmodules 
-fexclusive-builtin-modules` means, for instance (usually, later `-f` flags 
override earlier ones, so does this *turn off* modules for everything other 
than builtin headers?). Instead, we should use a command-line interface that 
more naturally / obviously composes with other flags.

It seems like the behavior you want here is precisely:

  `-fmodules -fno-implicit-module-maps -fmodule-map-file=/include/module.modulemap`

(The `-fmodules-validate-system-headers` part would only make sense for people 
using clang directly from their build area; installed / released versions of 
clang should not need this. Adding it makes your flag compose poorly with other 
uses of modules.)

I'd be happy with you adding a flag that is equivalent to 
`-fmodule-map-file=/include/module.modulemap`.

Also, it seems unlikely to me that we would ever turn this on by default. It 
will cause havoc for distributed builds, especially ones that run compilation 
actions in 'clean' environments, as (for instance) it will cause the complete 
set of intrinsics headers to be compiled into a module on every compile. So if 
this is your proposed solution for slow compiles using intrinsics headers, you 
should be aware that this is not a complete solution to that problem.


https://reviews.llvm.org/D25337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Bruno Cardoso Lopes via cfe-commits]

bruno added a comment.



> The long answer is that there is a history of problems regarding the 
> intrinsic files:
>  http://lists.llvm.org/pipermail/cfe-dev/2016-May/048837.html
>  http://lists.llvm.org/pipermail/cfe-dev/2016-September/050943.html
>  Mainly compatibility issues because MSVC makes all the intrinsics available 
> all the time, while in clang this is not always the case (On Windows the 
> different h files are not-included unless asked for explicitly since AVX512 
> was added which impacted compile-time).

Thinking a bit more about this: did you ever consider a solution where 
"immintrin.h" (or any similar other) would use "#include_next" and 
"__has_include_next" to point to a second level (user specified) immintrin.h, 
which then could use ifdefs to only include what's relevant?

> A suggestion was made to try and mitigate this by reducing the compile time 
> using modules (only for the x86 intrinsics). This patch aims at adding this 
> option. The new compile flag is just a milestone - if all goes well, we can 
> turn this on by default (Then we won't actually need to use a specific 
> compile flag, and we could always include all the intrinsic h files without 
> their long compile time).

The speedup sounds nice, but it seems awkward IMO to have such specific 
behavior for x86 intrinsics only.


https://reviews.llvm.org/D25337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Elad Cohen via cfe-commits]

eladcohen added a comment.

Hi Bruno,

The short answer is yes. Essentially it can be done, but we would actually like 
for all the users to always get this behavior, implicitly.

The long answer is that there is a history of problems regarding the intrinsic 
files:
http://lists.llvm.org/pipermail/cfe-dev/2016-May/048837.html
http://lists.llvm.org/pipermail/cfe-dev/2016-September/050943.html
Mainly compatibility issues because MSVC makes all the intrinsics available all 
the time, while in clang this is not always the case (On Windows the different 
h files are not-included unless asked for explicitly since AVX512 was added 
which impacted compile-time).
A suggestion was made to try and mitigate this by reducing the compile time 
using modules (only for the x86 intrinsics). This patch aims at adding this 
option. The new compile flag is just a milestone - if all goes well, we can 
turn this on by default (Then we won't actually need to use a specific compile 
flag, and we could always include all the intrinsic h files without their long 
compile time).


https://reviews.llvm.org/D25337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Bruno Cardoso Lopes via cfe-commits]

bruno added a reviewer: bruno.
bruno added a comment.

Hi Elad,

Is there any reason why you can't explicit model this in your build system by 
pre-building the intrinsics and pointing to a module cache path containing the 
pcm files? By doing that we don't need to have a specific compile flag.


https://reviews.llvm.org/D25337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25337: [Modules] Add a command line option for enabling the modules feature exclusively for the Clang builtins.

[Elad Cohen via cfe-commits]

eladcohen created this revision.
eladcohen added reviewers: hans, rnk, zvi, rsmith, chandlerc.
eladcohen added a subscriber: cfe-commits.

-fexclusive-builtin-modules enables the clang 'modules' feature exclusively for 
the clang intrinsic header files.

The end goal of this effort is to have this option on by default for x86 
targets so we could reduce the long compile time of the x86 intrinsic header 
files.


https://reviews.llvm.org/D25337

Files:
  docs/Modules.rst
  include/clang/Driver/Options.td
  lib/Driver/Tools.cpp
  test/CodeGen/3dnow-builtins.c
  test/CodeGen/adc-builtins.c
  test/CodeGen/adx-builtins.c
  test/CodeGen/attr-target-x86-mmx.c
  test/CodeGen/avx-builtins.c
  test/CodeGen/avx-cmp-builtins.c
  test/CodeGen/avx-shuffle-builtins.c
  test/CodeGen/avx2-builtins.c
  test/CodeGen/avx512bw-builtins.c
  test/CodeGen/avx512cdintrin.c
  test/CodeGen/avx512dq-builtins.c
  test/CodeGen/avx512er-builtins.c
  test/CodeGen/avx512f-builtins.c
  test/CodeGen/avx512ifma-builtins.c
  test/CodeGen/avx512ifmavl-builtins.c
  test/CodeGen/avx512pf-builtins.c
  test/CodeGen/avx512vbmi-builtins.c
  test/CodeGen/avx512vbmivl-builtin.c
  test/CodeGen/avx512vl-builtins.c
  test/CodeGen/avx512vlbw-builtins.c
  test/CodeGen/avx512vlcd-builtins.c
  test/CodeGen/avx512vldq-builtins.c
  test/CodeGen/bitscan-builtins.c
  test/CodeGen/bmi-builtins.c
  test/CodeGen/bmi2-builtins.c
  test/CodeGen/builtin-clflushopt.c
  test/CodeGen/f16c-builtins.c
  test/CodeGen/fma-builtins.c
  test/CodeGen/fma4-builtins.c
  test/CodeGen/fsgsbase-builtins.c
  test/CodeGen/lzcnt-builtins.c
  test/CodeGen/mmx-builtins.c
  test/CodeGen/pku.c
  test/CodeGen/popcnt-builtins.c
  test/CodeGen/prefetchw-builtins.c
  test/CodeGen/rd-builtins.c
  test/CodeGen/rdrand-builtins.c
  test/CodeGen/rtm-builtins.c
  test/CodeGen/sha-builtins.c
  test/CodeGen/sse-builtins.c
  test/CodeGen/sse2-builtins.c
  test/CodeGen/sse3-builtins.c
  test/CodeGen/sse41-builtins.c
  test/CodeGen/sse42-builtins.c
  test/CodeGen/sse4a-builtins.c
  test/CodeGen/ssse3-builtins.c
  test/CodeGen/target-builtin-error-2.c
  test/CodeGen/target-builtin-error.c
  test/CodeGen/target-builtin-noerror.c
  test/CodeGen/target-features-error-2.c
  test/CodeGen/tbm-builtins.c
  test/CodeGen/xop-builtins.c
  test/CodeGenCXX/mangle-ms-vector-types.cpp
  test/Driver/modules.m
  test/Driver/modules.mm
  test/Headers/x86-intrinsics-headers.c
  test/Headers/x86intrin-2.c
  test/Headers/x86intrin.c
  test/Headers/x86intrin.cpp
  test/lit.cfg

Index: test/lit.cfg
===
--- test/lit.cfg
+++ test/lit.cfg
@@ -268,6 +268,12 @@
 config.substitutions.append( ('%test_debuginfo', ' ' + config.llvm_src_root + '/utils/test_debuginfo.pl ') )
 config.substitutions.append( ('%itanium_abi_triple', makeItaniumABITriple(config.target_triple)) )
 config.substitutions.append( ('%ms_abi_triple', makeMSABITriple(config.target_triple)) )
+config.substitutions.append( ('%use_builtin_modules',
+  ' -fmodules -fmodule-map-file=' + 
+  getClangBuiltinIncludeDir(config.clang) +
+  '/module.modulemap '
+  '-fmodules-cache-path=%t_builtin_modules '
+  '-fmodules-validate-system-headers ' ))
 
 # The host triple might not be set, at least if we're compiling clang from
 # an already installed llvm.
Index: test/Headers/x86intrin.cpp
===
--- test/Headers/x86intrin.cpp
+++ test/Headers/x86intrin.cpp
@@ -1,4 +1,7 @@
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding %s -verify
+
+// RUN: %clang_cc1 %use_builtin_modules -fsyntax-only -ffreestanding %s -verify
+
 // expected-no-diagnostics
 
 #if defined(i386) || defined(__x86_64__)
Index: test/Headers/x86intrin.c
===
--- test/Headers/x86intrin.c
+++ test/Headers/x86intrin.c
@@ -1,6 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -fno-lax-vector-conversions %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -x c++ %s -verify
+
+// RUN: %clang_cc1 %use_builtin_modules -fsyntax-only -ffreestanding %s -verify
+// RUN: %clang_cc1 %use_builtin_modules -fsyntax-only -ffreestanding -fno-lax-vector-conversions %s -verify
+// RUN: %clang_cc1 %use_builtin_modules -fsyntax-only -ffreestanding -x c++ %s -verify
+
 // expected-no-diagnostics
 
 #if defined(i386) || defined(__x86_64__)
Index: test/Headers/x86intrin-2.c
===
--- test/Headers/x86intrin-2.c
+++ test/Headers/x86intrin-2.c
@@ -1,6 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -fno-lax-vector-conversions %s -verify
 // RUN: %clang_cc1 -fsyntax-only -ffreestanding -x c++ %s -verify
+
+// RUN: