[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D53891#4186894 , @tejohnson wrote:

> In D53891#4186892 , @nikic wrote:
>
>> In D53891#2116541 , @hans wrote:
>>
>>> Would be possible to add some documentation for this flag? From a quick 
>>> search it's not clear to me what it does and when one is supposed to use it.
>>
>> Ping on adding documentation.
>
> Thanks for the reminder, will do

Sent D145951 , ptal


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891

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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D53891#4186892 , @nikic wrote:

> In D53891#2116541 , @hans wrote:
>
>> Would be possible to add some documentation for this flag? From a quick 
>> search it's not clear to me what it does and when one is supposed to use it.
>
> Ping on adding documentation.

Thanks for the reminder, will do


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891

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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.
Herald added a project: All.

In D53891#2116541 , @hans wrote:

> Would be possible to add some documentation for this flag? From a quick 
> search it's not clear to me what it does and when one is supposed to use it.

Ping on adding documentation.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891

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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.
Herald added a subscriber: hiraditya.

Would be possible to add some documentation for this flag? From a quick search 
it's not clear to me what it does and when one is supposed to use it.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350949: [LTO] Add option to enable LTOUnit splitting, and 
disable unless needed (authored by tejohnson, committed by ).

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891

Files:
  cfe/trunk/include/clang/Basic/CodeGenOptions.def
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Driver/SanitizerArgs.h
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/lib/Driver/SanitizerArgs.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
  cfe/trunk/test/CodeGenCXX/no-lto-unit.cpp
  cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
  cfe/trunk/test/Driver/split-lto-unit.c

Index: cfe/trunk/include/clang/Driver/SanitizerArgs.h
===
--- cfe/trunk/include/clang/Driver/SanitizerArgs.h
+++ cfe/trunk/include/clang/Driver/SanitizerArgs.h
@@ -81,6 +81,7 @@
 
   bool requiresPIE() const;
   bool needsUnwindTables() const;
+  bool needsLTO() const;
   bool linkCXXRuntimes() const { return LinkCXXRuntimes; }
   bool hasCrossDsoCfi() const { return CfiCrossDso; }
   bool hasAnySanitizer() const { return !Sanitizers.empty(); }
Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1776,6 +1776,11 @@
   HelpText<"Enables whole-program vtable optimization. Requires -flto">;
 def fno_whole_program_vtables : Flag<["-"], "fno-whole-program-vtables">, Group,
   Flags<[CoreOption]>;
+def fsplit_lto_unit : Flag<["-"], "fsplit-lto-unit">, Group,
+  Flags<[CoreOption, CC1Option]>,
+  HelpText<"Enables splitting of the LTO unit.">;
+def fno_split_lto_unit : Flag<["-"], "fno-split-lto-unit">, Group,
+  Flags<[CoreOption]>;
 def fforce_emit_vtables : Flag<["-"], "fforce-emit-vtables">, Group,
 Flags<[CC1Option]>,
 HelpText<"Emits more virtual tables to improve devirtualization">;
Index: cfe/trunk/include/clang/Basic/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Basic/CodeGenOptions.def
+++ cfe/trunk/include/clang/Basic/CodeGenOptions.def
@@ -116,6 +116,10 @@
  ///< compile step.
 CODEGENOPT(LTOUnit, 1, 0) ///< Emit IR to support LTO unit features (CFI, whole
   ///< program vtable opt).
+CODEGENOPT(EnableSplitLTOUnit, 1, 0) ///< Enable LTO unit splitting to support
+ /// CFI and traditional whole program
+ /// devirtualization that require whole
+ /// program IR support.
 CODEGENOPT(IncrementalLinkerCompatible, 1, 0) ///< Emit an object file which can
   ///< be used with an incremental
   ///< linker.
Index: cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
+++ cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: %clang_cc1 -flto=thin -flto-unit -fsplit-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+// RUN: llvm-modextract -b -o - -n 1 %t | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=LTOUNIT
+// LTOUNIT: 
 
 // CHECK: @_ZTV1A = linkonce_odr
 class A {
Index: cfe/trunk/test/CodeGenCXX/no-lto-unit.cpp
===
--- cfe/trunk/test/CodeGenCXX/no-lto-unit.cpp
+++ cfe/trunk/test/CodeGenCXX/no-lto-unit.cpp
@@ -2,6 +2,8 @@
 // RUN: llvm-dis -o - %t | FileCheck %s
 // RUN: %clang_cc1 -flto=thin -flto-unit -fno-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-dis -o - %t | FileCheck %s
+// RUN: llvm-bcanalyzer -dump %t | FileCheck %s --check-prefix=NOLTOUNIT
+// NOLTOUNIT: 
 
 // CHECK-NOT: !type
 class A {
Index: cfe/trunk/test/Driver/split-lto-unit.c
===
--- cfe/trunk/test/Driver/split-lto-unit.c
+++ cfe/trunk/test/Driver/split-lto-unit.c
@@ -0,0 +1,10 @@
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fsplit-lto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang 

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson updated this revision to Diff 181317.
tejohnson marked 4 inline comments as done.
tejohnson added a comment.

Address comments.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Driver/Options.td
  include/clang/Driver/SanitizerArgs.h
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/thinlto-distributed-cfi-devirt.ll
  test/CodeGen/thinlto-distributed-cfi.ll
  test/CodeGenCXX/no-lto-unit.cpp
  test/CodeGenCXX/type-metadata-thinlto.cpp
  test/Driver/split-lto-unit.c

Index: test/Driver/split-lto-unit.c
===
--- /dev/null
+++ test/Driver/split-lto-unit.c
@@ -0,0 +1,10 @@
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fsplit-lto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=ERROR1 %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=ERROR2 %s
+
+// UNIT: "-fsplit-lto-unit"
+// NOUNIT-NOT: "-fsplit-lto-unit"
+// ERROR1: error: invalid argument '-fno-split-lto-unit' not allowed with '-fwhole-program-vtables'
+// ERROR2: error: invalid argument '-fno-split-lto-unit' not allowed with '-fsanitize=cfi'
Index: test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- test/CodeGenCXX/type-metadata-thinlto.cpp
+++ test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: %clang_cc1 -flto=thin -flto-unit -fsplit-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+// RUN: llvm-modextract -b -o - -n 1 %t | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=LTOUNIT
+// LTOUNIT: 
 
 // CHECK: @_ZTV1A = linkonce_odr
 class A {
Index: test/CodeGenCXX/no-lto-unit.cpp
===
--- test/CodeGenCXX/no-lto-unit.cpp
+++ test/CodeGenCXX/no-lto-unit.cpp
@@ -2,6 +2,8 @@
 // RUN: llvm-dis -o - %t | FileCheck %s
 // RUN: %clang_cc1 -flto=thin -flto-unit -fno-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-dis -o - %t | FileCheck %s
+// RUN: llvm-bcanalyzer -dump %t | FileCheck %s --check-prefix=NOLTOUNIT
+// NOLTOUNIT: 
 
 // CHECK-NOT: !type
 class A {
Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -2,7 +2,7 @@
 
 ; Backend test for distribute ThinLTO with CFI.
 
-; RUN: opt -thinlto-bc -o %t.o %s
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t.o %s
 
 ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \
 ; RUN:   -o %t2.index \
Index: test/CodeGen/thinlto-distributed-cfi-devirt.ll
===
--- test/CodeGen/thinlto-distributed-cfi-devirt.ll
+++ test/CodeGen/thinlto-distributed-cfi-devirt.ll
@@ -4,7 +4,7 @@
 ; It additionally enables -fwhole-program-vtables to get more information in
 ; TYPE_IDs of GLOBALVAL_SUMMARY_BLOCK.
 
-; RUN: opt -thinlto-bc -o %t.o %s
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t.o %s
 
 ; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
 ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -907,6 +907,7 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
   }
   Opts.LTOUnit = Args.hasFlag(OPT_flto_unit, OPT_fno_lto_unit, false);
+  Opts.EnableSplitLTOUnit = Args.hasArg(OPT_fsplit_lto_unit);
   if (Arg *A = Args.getLastArg(OPT_fthinlto_index_EQ)) {
 if (IK.getLanguage() != InputKind::LLVM_IR)
   Diags.Report(diag::err_drv_argument_only_allowed_with)
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -5226,6 +5226,17 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5112
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {

pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > Should this default to `WholeProgramVTables || Sanitize.needsLTO()` and 
> > > emit an error if the user passes the (for now) unsupported combinations 
> > > `-fno-split-lto-unit -fwhole-program-vtables` or `-fno-split-lto-unit 
> > > -fsanitize=cfi`?
> > I think the code below needs to stay as is to allow -fsplit-lto-unit to 
> > also enable the splitting even when the other options aren't on (not sure 
> > when that would be used outside of testing, but I'm assuming we want a way 
> > to force that on). 
> > 
> > But yes I think it makes sense to emit an error on those combinations (when 
> > my follow on patches go in then we would remove the error with 
> > -fno-split-lto-unit -fwhole-program-vtables).
> `-fsplit-lto-unit` is required when linking translation units compiled with 
> `-fsanitize=cfi` with translation units compiled without (the latter would 
> need the flag).
> 
> May I suggest simplifying this code as follows:
> ```
> bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
> bool SplitLTOUnit =
>  Args.hasFlag(options::OPT_fwhole_program_vtables,
> options::OPT_fno_whole_program_vtables, RequiresSplitLTOUnit);
> if (RequiresSplitLTOUnit && !SplitLTOUnit)
>error;
> if (SplitLTOUnit)
>   CmdArgs.push_back("-fsplit-lto-unit");
> ```
> 
Sorry, this was meant to be `OPT_fsplit_lto_unit` and `OPT_fno_split_lto_unit`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Peter Collingbourne via Phabricator via cfe-commits]

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: include/clang/Basic/CodeGenOptions.def:120
+CODEGENOPT(EnableSplitLTOUnit, 1, 0) ///< Enable LTO unit splitting to support
+/// CFI and tranditional whole program
+/// devirtulization that require whole

traditional



Comment at: include/clang/Basic/CodeGenOptions.def:121
+/// CFI and tranditional whole program
+/// devirtulization that require whole
+/// program IR support.

devirtualization



Comment at: lib/Driver/ToolChains/Clang.cpp:5112
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {

tejohnson wrote:
> pcc wrote:
> > Should this default to `WholeProgramVTables || Sanitize.needsLTO()` and 
> > emit an error if the user passes the (for now) unsupported combinations 
> > `-fno-split-lto-unit -fwhole-program-vtables` or `-fno-split-lto-unit 
> > -fsanitize=cfi`?
> I think the code below needs to stay as is to allow -fsplit-lto-unit to also 
> enable the splitting even when the other options aren't on (not sure when 
> that would be used outside of testing, but I'm assuming we want a way to 
> force that on). 
> 
> But yes I think it makes sense to emit an error on those combinations (when 
> my follow on patches go in then we would remove the error with 
> -fno-split-lto-unit -fwhole-program-vtables).
`-fsplit-lto-unit` is required when linking translation units compiled with 
`-fsanitize=cfi` with translation units compiled without (the latter would need 
the flag).

May I suggest simplifying this code as follows:
```
bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
bool SplitLTOUnit =
 Args.hasFlag(options::OPT_fwhole_program_vtables,
  options::OPT_fno_whole_program_vtables, RequiresSplitLTOUnit);
if (RequiresSplitLTOUnit && !SplitLTOUnit)
   error;
if (SplitLTOUnit)
  CmdArgs.push_back("-fsplit-lto-unit");
```



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

ping - @pcc do you have any more comments or can this be committed along with 
D53890 ?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson updated this revision to Diff 179305.
tejohnson added a comment.

Implement comment suggestions. Added test for new error. Also test changes for 
LLVM side changes in D53890 .


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Driver/Options.td
  include/clang/Driver/SanitizerArgs.h
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/thinlto-distributed-cfi-devirt.ll
  test/CodeGen/thinlto-distributed-cfi.ll
  test/CodeGenCXX/no-lto-unit.cpp
  test/CodeGenCXX/type-metadata-thinlto.cpp
  test/Driver/split-lto-unit.c

Index: test/Driver/split-lto-unit.c
===
--- /dev/null
+++ test/Driver/split-lto-unit.c
@@ -0,0 +1,10 @@
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fsplit-lto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=ERROR1 %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=ERROR2 %s
+
+// UNIT: "-fsplit-lto-unit"
+// NOUNIT-NOT: "-fsplit-lto-unit"
+// ERROR1: error: invalid argument '-fno-split-lto-unit' not allowed with '-fwhole-program-vtables'
+// ERROR2: error: invalid argument '-fno-split-lto-unit' not allowed with '-fsanitize=cfi'
Index: test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- test/CodeGenCXX/type-metadata-thinlto.cpp
+++ test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: %clang_cc1 -flto=thin -flto-unit -fsplit-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+// RUN: llvm-modextract -b -o - -n 1 %t | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=LTOUNIT
+// LTOUNIT: 
 
 // CHECK: @_ZTV1A = linkonce_odr
 class A {
Index: test/CodeGenCXX/no-lto-unit.cpp
===
--- test/CodeGenCXX/no-lto-unit.cpp
+++ test/CodeGenCXX/no-lto-unit.cpp
@@ -2,6 +2,8 @@
 // RUN: llvm-dis -o - %t | FileCheck %s
 // RUN: %clang_cc1 -flto=thin -flto-unit -fno-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-dis -o - %t | FileCheck %s
+// RUN: llvm-bcanalyzer -dump %t | FileCheck %s --check-prefix=NOLTOUNIT
+// NOLTOUNIT: 
 
 // CHECK-NOT: !type
 class A {
Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -2,7 +2,7 @@
 
 ; Backend test for distribute ThinLTO with CFI.
 
-; RUN: opt -thinlto-bc -o %t.o %s
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t.o %s
 
 ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \
 ; RUN:   -o %t2.index \
Index: test/CodeGen/thinlto-distributed-cfi-devirt.ll
===
--- test/CodeGen/thinlto-distributed-cfi-devirt.ll
+++ test/CodeGen/thinlto-distributed-cfi-devirt.ll
@@ -4,7 +4,7 @@
 ; It additionally enables -fwhole-program-vtables to get more information in
 ; TYPE_IDs of GLOBALVAL_SUMMARY_BLOCK.
 
-; RUN: opt -thinlto-bc -o %t.o %s
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t.o %s
 
 ; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
 ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -906,6 +906,7 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
   }
   Opts.LTOUnit = Args.hasFlag(OPT_flto_unit, OPT_fno_lto_unit, false);
+  Opts.EnableSplitLTOUnit = Args.hasArg(OPT_fsplit_lto_unit);
   if (Arg *A = Args.getLastArg(OPT_fthinlto_index_EQ)) {
 if (IK.getLanguage() != InputKind::LLVM_IR)
   Diags.Report(diag::err_drv_argument_only_allowed_with)
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -5206,6 +5206,21 @@
 CmdArgs.push_back("-fwhole-program-vtables");
  

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked 2 inline comments as done.
tejohnson added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:831
+  *OS, CodeGenOpts.EmitLLVMUseLists, EmitLTOSummary,
+  /*EmitModuleHash=*/false));
 }

pcc wrote:
> Why add this argument here (and below)?
I think this was leftover from an earlier version where I had added a new 
parameter. Will revert this change (here and below).



Comment at: lib/Driver/ToolChains/Clang.cpp:5112
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {

pcc wrote:
> Should this default to `WholeProgramVTables || Sanitize.needsLTO()` and emit 
> an error if the user passes the (for now) unsupported combinations 
> `-fno-split-lto-unit -fwhole-program-vtables` or `-fno-split-lto-unit 
> -fsanitize=cfi`?
I think the code below needs to stay as is to allow -fsplit-lto-unit to also 
enable the splitting even when the other options aren't on (not sure when that 
would be used outside of testing, but I'm assuming we want a way to force that 
on). 

But yes I think it makes sense to emit an error on those combinations (when my 
follow on patches go in then we would remove the error with -fno-split-lto-unit 
-fwhole-program-vtables).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

You can't emit the type tests by default with `-flto=thin` because not all 
programs adhere to the conditions described in `LTOVisibility.rst` and we can't 
silently break those programs; it is something that they will need to 
explicitly opt in to. We should be able to emit the `!type` annotations by 
default, just not the type tests.




Comment at: lib/CodeGen/BackendUtil.cpp:831
+  *OS, CodeGenOpts.EmitLLVMUseLists, EmitLTOSummary,
+  /*EmitModuleHash=*/false));
 }

Why add this argument here (and below)?



Comment at: lib/Driver/ToolChains/Clang.cpp:5112
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {

Should this default to `WholeProgramVTables || Sanitize.needsLTO()` and emit an 
error if the user passes the (for now) unsupported combinations 
`-fno-split-lto-unit -fwhole-program-vtables` or `-fno-split-lto-unit 
-fsanitize=cfi`?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53891/new/

https://reviews.llvm.org/D53891



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