[PATCH] D50882: [ThinLTO] Correct documentation on default number of threads

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: dexonsmith, steven_wu, eraman, inglorion, mehdi_amini.

The number of threads used for ThinLTO backend parallelism was
dropped to the number of cores in r284618 to avoid oversubscribing
physical cores due to hyperthreading. This updates the documentation
to reflect that change.

Fixes PR38610.


Repository:
  rC Clang

https://reviews.llvm.org/D50882

Files:
  docs/ThinLTO.rst


Index: docs/ThinLTO.rst
===
--- docs/ThinLTO.rst
+++ docs/ThinLTO.rst
@@ -105,7 +105,9 @@
 ---
 .. _parallelism:
 
-By default, the ThinLTO link step will launch up to
+By default, the ThinLTO link step will launch as many
+threads in parallel as there are cores. If the number of
+cores can't be computed for the architecture, then it will launch
 ``std::thread::hardware_concurrency`` number of threads in parallel.
 For machines with hyper-threading, this is the total number of
 virtual cores. For some applications and machine configurations this


Index: docs/ThinLTO.rst
===
--- docs/ThinLTO.rst
+++ docs/ThinLTO.rst
@@ -105,7 +105,9 @@
 ---
 .. _parallelism:
 
-By default, the ThinLTO link step will launch up to
+By default, the ThinLTO link step will launch as many
+threads in parallel as there are cores. If the number of
+cores can't be computed for the architecture, then it will launch
 ``std::thread::hardware_concurrency`` number of threads in parallel.
 For machines with hyper-threading, this is the total number of
 virtual cores. For some applications and machine configurations this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50882: [ThinLTO] Correct documentation on default number of threads

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340021: [ThinLTO] Correct documentation on default number of 
threads (authored by tejohnson, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50882?vs=161161&id=161223#toc

Repository:
  rC Clang

https://reviews.llvm.org/D50882

Files:
  docs/ThinLTO.rst


Index: docs/ThinLTO.rst
===
--- docs/ThinLTO.rst
+++ docs/ThinLTO.rst
@@ -105,7 +105,9 @@
 ---
 .. _parallelism:
 
-By default, the ThinLTO link step will launch up to
+By default, the ThinLTO link step will launch as many
+threads in parallel as there are cores. If the number of
+cores can't be computed for the architecture, then it will launch
 ``std::thread::hardware_concurrency`` number of threads in parallel.
 For machines with hyper-threading, this is the total number of
 virtual cores. For some applications and machine configurations this


Index: docs/ThinLTO.rst
===
--- docs/ThinLTO.rst
+++ docs/ThinLTO.rst
@@ -105,7 +105,9 @@
 ---
 .. _parallelism:
 
-By default, the ThinLTO link step will launch up to
+By default, the ThinLTO link step will launch as many
+threads in parallel as there are cores. If the number of
+cores can't be computed for the architecture, then it will launch
 ``std::thread::hardware_concurrency`` number of threads in parallel.
 For machines with hyper-threading, this is the total number of
 virtual cores. For some applications and machine configurations this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46700: [ThinLTO] Add testing of new summary index format to a couple CFI tests

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: eraman, inglorion, mehdi_amini.

Adds testing of combined index summary entries in disassembly format
to CFI tests that were already testing the bitcode format.

Depends on https://reviews.llvm.org/D46699.


Repository:
  rC Clang

https://reviews.llvm.org/D46700

Files:
  test/CodeGen/thinlto-distributed-cfi-devirt.ll
  test/CodeGen/thinlto-distributed-cfi.ll


Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: ___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46700: [ThinLTO] Add testing of new summary index format to a couple CFI tests

[Teresa Johnson via Phabricator via cfe-commits]

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

Update tests for changes to https://reviews.llvm.org/D46699


Repository:
  rC Clang

https://reviews.llvm.org/D46700

Files:
  test/CodeGen/thinlto-distributed-cfi-devirt.ll
  test/CodeGen/thinlto-distributed-cfi.ll


Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: ___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46700: [ThinLTO] Add testing of new summary index format to a couple CFI tests

[Teresa Johnson via Phabricator via cfe-commits]

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

Update tests so they won't get bot failures (don't try to match path, module
hash).

Ping - this can go in now that https://reviews.llvm.org/D46699 is in.


Repository:
  rC Clang

https://reviews.llvm.org/D46700

Files:
  test/CodeGen/thinlto-distributed-cfi-devirt.ll
  test/CodeGen/thinlto-distributed-cfi.ll


Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: ___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38517: Enabling new pass manager in LTO (and thinLTO) link step via -fexperimental-new-pass-manager option

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Thanks for adding this. Please add a test like the one for -ffunction-sections 
(tools/clang/test/Driver/gold-lto-sections.c).


Repository:
  rL LLVM

https://reviews.llvm.org/D38517



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

[PATCH] D38517: Enabling new pass manager in LTO (and thinLTO) link step via -fexperimental-new-pass-manager option

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM
thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D38517



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

[PATCH] D51198: [LTO] Fix -save-temps with LTO and unnamed globals.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Thanks. Can you fix the same code in EmitAssemblyWithNewPassManager?


Repository:
  rC Clang

https://reviews.llvm.org/D51198



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

[PATCH] D51198: [LTO] Fix -save-temps with LTO and unnamed globals.

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D51198



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

[PATCH] D42995: [ThinLTO] Ignore object files with empty ThinLTO index

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

> Empty ThinLTOIndexFile signals that we don't need this module during
>  linking.

Not the only case actually. We now also pass an empty index file when we want 
to compile the bitcode down to object without applying any LTO optimization 
(there are a few cases where we decide we want to turn off LTO optimizations 
for some links), and this is currently relying on being able to pass /dev/null 
for the index file that would be broken by this change.

> So we should not run ThinLTO backend even if it contains the
>  ThinLTO module. Backend may fail because of lack of necessary
>  information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() 
is called, EmitBackendOutput() is called which passes 
/*IgnoreEmptyThinLTOIndexFile*/true to  getModuleSummaryIndexForFile, which 
would cause it to return nullptr if the index file is empty.  Back in 
EmitBackendOutput(), if the combined index is null we will skip ThinLTO 
compilation and fall back to normal compilation.


https://reviews.llvm.org/D42995



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

[PATCH] D42995: [ThinLTO] Ignore object files with empty ThinLTO index

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D42995#1000155, @vitalybuka wrote:

> In https://reviews.llvm.org/D42995#125, @tejohnson wrote:
>
> > > Empty ThinLTOIndexFile signals that we don't need this module during
> > >  linking.
> >
> > Not the only case actually. We now also pass an empty index file when we 
> > want to compile the bitcode down to object without applying any LTO 
> > optimization (there are a few cases where we decide we want to turn off LTO 
> > optimizations for some links), and this is currently relying on being able 
> > to pass /dev/null for the index file that would be broken by this change.
>
>
> I'd expect this should be done by indexing and content is already in the 
> merged object file.
>  Not sure how to reproduce this. I've build some large targets and I never 
> seen this.


This is done in bazel, not in gold.  See below.

> 
> 
>> 
>> 
>>> So we should not run ThinLTO backend even if it contains the
>>>  ThinLTO module. Backend may fail because of lack of necessary
>>>  information which should be provided by ThinLTOIndex.
>> 
>> This shouldn't happen - are you seeing cases where we fail? After 
>> loadModule() is called, EmitBackendOutput() is called which passes 
>> /*IgnoreEmptyThinLTOIndexFile*/true to  getModuleSummaryIndexForFile, which 
>> would cause it to return nullptr if the index file is empty.  Back in 
>> EmitBackendOutput(), if the combined index is null we will skip ThinLTO 
>> compilation and fall back to normal compilation.
> 
> I don't see for regular compilation, but I see for  for CFI. Backend will not 
> be able to process llvm.type.test without TypeIdMap from index and it will 
> crash in "Instruction Select"

@pcc and I already discussed this a little bit in the context of bazel. I will 
point you to that discussion separately.


https://reviews.llvm.org/D42995



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

[PATCH] D42995: [ThinLTO] Allow indexing to request backend to ignore the module

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1176
+  // Output of this module is not needed, but build system may not know
+  // that. So we need to generated empty valid object file.
+  EmptyModule = llvm::make_unique("empty", M->getContext());

s/generated/generate an/



Comment at: clang/test/CodeGen/thinlto-distributed-backend-skip.ll:3
+
+; Check that ThinLTO backed respects "SkipModule" flag which can be set by
+; indexing.

s/backed/backend/



Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:685
 
+  /// Indicates that corresponding module should be skipped.
+  bool SkipModule = false;

Need to document clearly that this only applies to the distributed index case, 
and what it means to "skip" the module (i.e. skip the backend compilation and 
generate an empty module instead). 

Maybe rename to something like SkipModuleDistributedBackend or something else 
very explicit to avoid confusion?



Comment at: llvm/tools/gold/gold-plugin.cpp:819
+  const std::string &NewPrefix,
+  bool Skip) {
   std::string NewModulePath =

Rename Skip to SkipModule to match the above comment documenting parameter



Comment at: llvm/tools/gold/gold-plugin.cpp:890
   addModule(*Lto, F, View, ObjFilename.first->first());
+else {
+  ObjFilename.first->second = true;

Think this should be "else if (options::thinlto_index_only) {"
i.e. we don't need/want to write these when we're in single process mode.



Comment at: llvm/tools/gold/gold-plugin.cpp:892
+  ObjFilename.first->second = true;
+  writeEmptyDistributedBuildOutputs(Identifier, OldPrefix, NewPrefix, 
true);
+}

Document constant parameters here and in other places. e.g. "/* SkipModule= */ 
true"


https://reviews.llvm.org/D42995



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

[PATCH] D42995: [ThinLTO] Allow indexing to request backend to ignore the module

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D42995



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

[PATCH] D34055: Be more strict when checking the -flto option value

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Make sure you add cfe-commits to the mailing list for clang changes (and 
llvm-commits for llvm changes, I accidentally added that one first and then 
fixed it), since all patches should go to the full mailing list.

This fix looks correct to me. Please add a test case though. Perhaps there 
should be an error if the value is something other than "thin" or "lto"? E.g. 
see the error that will get emitted if the value is not one of those in 
clang/lib/Driver/Driver.cpp.


https://reviews.llvm.org/D34055



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

[PATCH] D34055: Be more strict when checking the -flto option value

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D34055#777005, @yamaguchi wrote:

> I think we don't need additional testcase, because this is non-functional 
> change. Driver.cpp will emit error if value was not "thin" nor "full". This 
> testcase is at clang/test/CodeGen/thinlto-backend-option.ll.


That test case is checking that -flto=thin is accepted by the driver. I don't 
see it checking for an unknown -flto= value. But in any case, you can go 
through the code you are fixing here without hitting the driver check by 
invoking clang -cc1 (%clang_cc1 in the test case). I just confirmed that 
-flto=thinfoo is flagged as an error by clang (when it goes through the 
driver), but is silently accepted if I pass directly to clang -cc1. So this is 
definitely a bug fix and it would be good to check for invalid -flto values and 
given an error,  then have a test that uses %clang_cc1 and confirms that an 
unknown -flto= value gets that error.


https://reviews.llvm.org/D34055



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

[PATCH] D34055: Be more strict when checking the -flto option value

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM, thanks for the fix!


https://reviews.llvm.org/D34055



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

[PATCH] D34546: docs: Add documentation for the ThinLTO cache pruning policy string.

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D34546



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

[PATCH] D47597: IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto-index.

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM thanks!


https://reviews.llvm.org/D47597



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

[PATCH] D46700: [ThinLTO] Add testing of new summary index format to a couple CFI tests

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.
Herald added a subscriber: steven_wu.

ping


Repository:
  rC Clang

https://reviews.llvm.org/D46700



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

[PATCH] D46700: [ThinLTO] Add testing of new summary index format to a couple CFI tests

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333966: [ThinLTO] Add testing of new summary index format to 
a couple CFI tests (authored by tejohnson, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D46700

Files:
  cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll


Index: cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
===
--- cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
+++ cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: Index: cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
===
--- cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
+++ cfe/trunk/test/CodeGen/thinlto-distributed-cfi.ll
@@ -20,6 +20,11 @@
 ; CHECK: blob data = '_ZTS1A'
 ; CHECK-LABEL: ___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47906: [ThinLTO] Add testing of summary index parsing to a couple CFI tests

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added reviewers: pcc, dexonsmith, mehdi_amini.
Herald added subscribers: steven_wu, eraman, inglorion.

Changes to some clang side tests to go with the summary parsing patch.

Depends on https://reviews.llvm.org/D47905.


Repository:
  rC Clang

https://reviews.llvm.org/D47906

Files:
  test/CodeGen/thinlto-distributed-cfi-devirt.ll
  test/CodeGen/thinlto-distributed-cfi.ll


Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -21,6 +21,8 @@
 ; CHECK-LABEL: Index: test/CodeGen/thinlto-distributed-cfi.ll
===
--- test/CodeGen/thinlto-distributed-cfi.ll
+++ test/CodeGen/thinlto-distributed-cfi.ll
@@ -21,6 +21,8 @@
 ; CHECK-LABEL: ___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34156: [LTO] Enable module summary emission by default for regular LTO

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson accepted this revision.
tejohnson added a comment.

LGTM with following suggestions.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:776
+  bool EmitLTOSummary =
+  (CodeGenOpts.PrepareForLTO && !CodeGenOpts.PrepareForThinLTO &&
+   llvm::Triple(TheModule->getTargetTriple()).getVendor() !=

Perhaps sink all this code into the use site of EmitLTOSummary below, 
especially because you don't need to check !CodeGenOpts.PrepareForThinLTO.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:998
+  bool EmitLTOSummary =
+  (CodeGenOpts.PrepareForLTO && !CodeGenOpts.PrepareForThinLTO &&
+   llvm::Triple(TheModule->getTargetTriple()).getVendor() !=

Ditto here.


https://reviews.llvm.org/D34156



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

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: dexonsmith, steven_wu, inglorion, mehdi_amini.

Currently, -flto-unit is specified whenever LTO options are used
(unless using the old LTO API). This causes vtable defs to be processed
using regular LTO, which is needed for CFI and whole program vtable
optimizations, since they need to modify the vtables in a whole program
manner.

However, this causes non-negligible overhead due to the regular
LTO processing. Since this isn't needed when not using CFI or
-fwhole-program-vtables, only enable -flto-unit in those cases.
Otherwise all ThinLTO compiles pay the overhead, even when not needed.


Repository:
  rC Clang

https://reviews.llvm.org/D53524

Files:
  include/clang/Driver/SanitizerArgs.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/lto-unit.c


Index: test/Driver/lto-unit.c
===
--- test/Driver/lto-unit.c
+++ test/Driver/lto-unit.c
@@ -1,9 +1,9 @@
-// RUN: %clang -target x86_64-unknown-linux -### %s -flto=full 2>&1 | 
FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 2>&1 | 
FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=full 2>&1 | 
FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=thin 2>&1 | 
FileCheck --check-prefix=NOUNIT %s
-// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=full 2>&1 | FileCheck 
--check-prefix=UNIT %s
-// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=thin 2>&1 | FileCheck 
--check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=full 
-fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 
-fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=full 
-fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=thin 
-fwhole-program-vtables 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=full 
-fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=thin 
-fwhole-program-vtables 2>&1 | FileCheck --check-prefix=NOUNIT %s
 
 // UNIT: "-flto-unit"
 // NOUNIT-NOT: "-flto-unit"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3364,21 +3364,6 @@
 // the use-list order, since serialization to bitcode is part of the flow.
 if (JA.getType() == types::TY_LLVM_BC)
   CmdArgs.push_back("-emit-llvm-uselists");
-
-// Device-side jobs do not support LTO.
-bool isDeviceOffloadAction = !(JA.isDeviceOffloading(Action::OFK_None) ||
-   JA.isDeviceOffloading(Action::OFK_Host));
-
-if (D.isUsingLTO() && !isDeviceOffloadAction) {
-  Args.AddLastArg(CmdArgs, options::OPT_flto, options::OPT_flto_EQ);
-
-  // The Darwin and PS4 linkers currently use the legacy LTO API, which
-  // does not support LTO unit features (CFI, whole program vtable opt)
-  // under ThinLTO.
-  if (!(RawTriple.isOSDarwin() || RawTriple.isPS4()) ||
-  D.getLTOMode() == LTOK_Full)
-CmdArgs.push_back("-flto-unit");
-}
   }
 
   if (const Arg *A = Args.getLastArg(options::OPT_fthinlto_index_EQ)) {
@@ -4980,6 +4965,25 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  // Device-side jobs do not support LTO.
+  bool isDeviceOffloadAction = !(JA.isDeviceOffloading(Action::OFK_None) ||
+ JA.isDeviceOffloading(Action::OFK_Host));
+
+  if (D.isUsingLTO() &&
+  (isa(JA) || isa(JA)) &&
+  !isDeviceOffloadAction) {
+Args.AddLastArg(CmdArgs, options::OPT_flto, options::OPT_flto_EQ);
+
+// Enable LTO unit if need for CFI or whole program vtable optimization.
+// The Darwin and PS4 linkers currently use the legacy LTO API, which
+// does not support LTO unit features (CFI, whole program vtable opt)
+// under ThinLTO.
+bool SupportsLTOUnit = !(RawTriple.isOSDarwin() || RawTriple.isPS4()) ||
+   D.getLTOMode() == LTOK_Full;
+if ((WholeProgramVTables || Sanitize.needsLTO()) && SupportsLTOUnit)
+  CmdArgs.push_back("-flto-unit");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fexperimental_isel,
options::OPT_fno_experimental_isel)) {
 CmdArgs.push_back("-mllvm");
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -207,6 +207,10 @@
   return Sanitizers.Mask & NeedsUnwindTables;
 }
 

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53524#1271357, @pcc wrote:

> The reason why LTO unit is always enabled is so that you can link translation 
> units compiled with `-fsanitize=cfi` and/or `-fwhole-program-vtables` against 
> translation units compiled without CFI/WPD. With this change we will see 
> miscompiles in the translation units compiled with CFI/WPD if they use 
> vtables in the translation units compiled without CFI/WPD. If we really need 
> this option I think it should be an opt out.


Is there an important use case for support thing mixing and matching? The issue 
is that it comes at a cost to all ThinLTO compiles for codes with vtables by 
requiring them all to process IR during the thin link. Can we detect that TUs 
compiled with -flto-unit are being mixed with those not built without 
-flto-unit at the thin link time and issue an error?


Repository:
  rC Clang

https://reviews.llvm.org/D53524



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

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53524#1271387, @tejohnson wrote:

> In https://reviews.llvm.org/D53524#1271357, @pcc wrote:
>
> > The reason why LTO unit is always enabled is so that you can link 
> > translation units compiled with `-fsanitize=cfi` and/or 
> > `-fwhole-program-vtables` against translation units compiled without 
> > CFI/WPD. With this change we will see miscompiles in the translation units 
> > compiled with CFI/WPD if they use vtables in the translation units compiled 
> > without CFI/WPD. If we really need this option I think it should be an opt 
> > out.
>
>
> Is there an important use case for support thing mixing and matching? The 
> issue is that it comes at a cost to all ThinLTO compiles for codes with 
> vtables by requiring them all to process IR during the thin link.


Ping on the question of why this mode needs to be default. If it was a matter 
of a few percent overhead that would be one thing, but we're talking a *huge* 
overhead (as noted off-patch for my app I'm seeing >20x thin link time 
currently, and with improvements to the hashing to always get successful 
splitting we could potentially get down to closer to 2x - still a big 
overhead). This kind of overhead should be opt-in. The average ThinLTO user is 
not going to realize they are paying a big overhead because CFI is always 
pre-enabled.

> Can we detect that TUs compiled with -flto-unit are being mixed with those 
> not built without -flto-unit at the thin link time and issue an error?

This would be doable pretty easily. E.g. add a flag at the index level that the 
module would have been split but wasn't. Users who get the error and want to 
support always-enabled CFI could opt in via -flto-unit.


Repository:
  rC Clang

https://reviews.llvm.org/D53524



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

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53524#1276038, @pcc wrote:

> In https://reviews.llvm.org/D53524#1274505, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D53524#1271387, @tejohnson wrote:
> >
> > > In https://reviews.llvm.org/D53524#1271357, @pcc wrote:
> > >
> > > > The reason why LTO unit is always enabled is so that you can link 
> > > > translation units compiled with `-fsanitize=cfi` and/or 
> > > > `-fwhole-program-vtables` against translation units compiled without 
> > > > CFI/WPD. With this change we will see miscompiles in the translation 
> > > > units compiled with CFI/WPD if they use vtables in the translation 
> > > > units compiled without CFI/WPD. If we really need this option I think 
> > > > it should be an opt out.
> > >
> > >
> > > Is there an important use case for support thing mixing and matching? The 
> > > issue is that it comes at a cost to all ThinLTO compiles for codes with 
> > > vtables by requiring them all to process IR during the thin link.
> >
> >
> > Ping on the question of why this mode needs to be default. If it was a 
> > matter of a few percent overhead that would be one thing, but we're talking 
> > a *huge* overhead (as noted off-patch for my app I'm seeing >20x thin link 
> > time currently, and with improvements to the hashing to always get 
> > successful splitting we could potentially get down to closer to 2x - still 
> > a big overhead). This kind of overhead should be opt-in. The average 
> > ThinLTO user is not going to realize they are paying a big overhead because 
> > CFI is always pre-enabled.
>
>
> Well, the intent was always that the overhead would be minimal, which is why 
> things are set up the way that they are. But it doesn't sound like anyone is 
> going to have the time to fully address the performance problems that you've 
> seen any time soon, so maybe it would be fine to introduce the -flto-unit 
> flag. I guess we can always change the flag so that it has no effect if/when 
> the performance problem is addressed.


Just to clarify, since there is already a -flto-unit flag: it is currently a 
cc1 flag, did you want it made into a driver option as well?

> 
> 
>>> Can we detect that TUs compiled with -flto-unit are being mixed with those 
>>> not built without -flto-unit at the thin link time and issue an error?
>> 
>> This would be doable pretty easily. E.g. add a flag at the index level that 
>> the module would have been split but wasn't. Users who get the error and 
>> want to support always-enabled CFI could opt in via -flto-unit.
> 
> Yes. I don't think we should make a change like this unless there is 
> something like that in place, though. The documentation (LTOVisibility.rst) 
> needs to be updated too.

Ok, let me work on that now and we can get that in before this one.


Repository:
  rC Clang

https://reviews.llvm.org/D53524



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

[PATCH] D53891: [LTO] Pass down LTOUnit codegen flag to bitcode writer

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: cfe-commits, dexonsmith, steven_wu, eraman, 
inglorion, mehdi_amini.

Clang side patch for recording the LTOUnit flag in the index.

Depends on https://reviews.llvm.org/D53890.


Repository:
  rC Clang

https://reviews.llvm.org/D53891

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGenCXX/no-lto-unit.cpp
  test/CodeGenCXX/type-metadata-thinlto.cpp


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: 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: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -808,7 +808,7 @@
   return;
   }
   PerModulePasses.add(createWriteThinLTOBitcodePass(
-  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr));
+  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr, CodeGenOpts.LTOUnit));
 } else {
   // Emit a module summary by default for Regular LTO except for ld64
   // targets
@@ -822,7 +822,8 @@
 
   PerModulePasses.add(
   createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-  EmitLTOSummary));
+  EmitLTOSummary, /*EmitModuleHash=*/false,
+  CodeGenOpts.LTOUnit));
 }
 break;
 
@@ -1042,8 +1043,8 @@
 if (!ThinLinkOS)
   return;
   }
-  MPM.addPass(ThinLTOBitcodeWriterPass(*OS, ThinLinkOS ? &ThinLinkOS->os()
-   : nullptr));
+  MPM.addPass(ThinLTOBitcodeWriterPass(
+  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr, CodeGenOpts.LTOUnit));
 } else {
   // Emit a module summary by default for Regular LTO except for ld64
   // targets
@@ -1056,7 +1057,8 @@
 TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 
   MPM.addPass(BitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-EmitLTOSummary));
+EmitLTOSummary, /*EmitModuleHash=*/false,
+CodeGenOpts.LTOUnit));
 }
 break;
 


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: 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: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -808,7 +808,7 @@
   return;
   }
   PerModulePasses.add(createWriteThinLTOBitcodePass(
-  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr));
+  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr, CodeGenOpts.LTOUnit));
 } else {
   // Emit a module summary by default for Regular LTO except for ld64
   // targets
@@ -822,7 +822,8 @@
 
   PerModulePasses.add(
   createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-  

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53524#1279288, @tejohnson wrote:

> In https://reviews.llvm.org/D53524#1276038, @pcc wrote:
>
> > In https://reviews.llvm.org/D53524#1274505, @tejohnson wrote:
> >
> > > In https://reviews.llvm.org/D53524#1271387, @tejohnson wrote:
> > >
> > > > Can we detect that TUs compiled with -flto-unit are being mixed with 
> > > > those not built without -flto-unit at the thin link time and issue an 
> > > > error?
> > >
> > >
> > > This would be doable pretty easily. E.g. add a flag at the index level 
> > > that the module would have been split but wasn't. Users who get the error 
> > > and want to support always-enabled CFI could opt in via -flto-unit.
> >
> >
> > Yes. I don't think we should make a change like this unless there is 
> > something like that in place, though. The documentation (LTOVisibility.rst) 
> > needs to be updated too.
>
>
> Ok, let me work on that now and we can get that in before this one.


Mailed https://reviews.llvm.org/D53890 for this part.


Repository:
  rC Clang

https://reviews.llvm.org/D53524



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

[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 -targe

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

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

Address comments:
Promote -flto-unit to clang driver option (and test it)
Adjust LTOVisibility.rst to reflect change of default and new option.


Repository:
  rC Clang

https://reviews.llvm.org/D53524

Files:
  docs/LTOVisibility.rst
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Driver/SanitizerArgs.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/lto-unit.c

Index: test/Driver/lto-unit.c
===
--- test/Driver/lto-unit.c
+++ test/Driver/lto-unit.c
@@ -1,9 +1,18 @@
-// RUN: %clang -target x86_64-unknown-linux -### %s -flto=full 2>&1 | FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 2>&1 | FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=full 2>&1 | FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
-// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=full 2>&1 | FileCheck --check-prefix=UNIT %s
-// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=full -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=full -flto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -flto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=full -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=thin -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=full -flto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -flto=thin -flto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=full -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=thin -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=full -flto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -flto=thin -flto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
 
 // UNIT: "-flto-unit"
 // NOUNIT-NOT: "-flto-unit"
+
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto-unit 2>&1 | FileCheck --check-prefix=NO-LTO %s
+// NO-LTO: invalid argument '-flto-unit' only allowed with '-flto'
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3364,21 +3364,6 @@
 // the use-list order, since serialization to bitcode is part of the flow.
 if (JA.getType() == types::TY_LLVM_BC)
   CmdArgs.push_back("-emit-llvm-uselists");
-
-// Device-side jobs do not support LTO.
-bool isDeviceOffloadAction = !(JA.isDeviceOffloading(Action::OFK_None) ||
-   JA.isDeviceOffloading(Action::OFK_Host));
-
-if (D.isUsingLTO() && !isDeviceOffloadAction) {
-  Args.AddLastArg(CmdArgs, options::OPT_flto, options::OPT_flto_EQ);
-
-  // The Darwin and PS4 linkers currently use the legacy LTO API, which
-  // does not support LTO unit features (CFI, whole program vtable opt)
-  // under ThinLTO.
-  if (!(RawTriple.isOSDarwin() || RawTriple.isPS4()) ||
-  D.getLTOMode() == LTOK_Full)
-CmdArgs.push_back("-flto-unit");
-}
   }
 
   if (const Arg *A = Args.getLastArg(options::OPT_fthinlto_index_EQ)) {
@@ -4980,6 +4965,31 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  bool LTOUnit = Args.hasArg(options::OPT_flto_unit);
+  if (LTOUnit && !D.isUsingLTO())
+D.Diag(diag::err_drv_argument_only_allowed_with) << "-flto-unit"
+ << "-flto";
+
+  // Device-side jobs do not support LTO.
+  bool isDeviceOffloadAction = !(JA.isDeviceOffloading(Action::OFK_None) ||
+ JA.isDeviceOffloading(Action::OFK_Host));
+
+  if (D.isUsingLTO() &&
+  (isa(JA) || isa(JA)) &&
+  !isDeviceOffloadAction) {
+Args.AddLastArg(CmdArgs, options::OPT_flto, options::OPT_flto_EQ);
+
+// Enable LTO unit if need for CFI or whole program vtable optimization.
+// The Darwin and PS4 linkers currently use the legacy LTO API, which
+// does not support LTO unit features (CFI, who

[PATCH] D53891: [LTO] Pass down LTOUnit codegen flag to bitcode writer

[Teresa Johnson via Phabricator via cfe-commits]

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

- As discussed off-patch, will use a new, separate option to control this

splitting, here I am using -f[no]split-lto-unit.

- Switch the default of splitting lto units to off by default, unless

compiled with CFI or -fwhole-program-vtables.

- I'll update the patch title and summary once we converge on flag/behavior.


Repository:
  rC Clang

https://reviews.llvm.org/D53891

Files:
  include/clang/Driver/Options.td
  include/clang/Driver/SanitizerArgs.h
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  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,8 @@
+// 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=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=UNIT %s
+
+// UNIT: "-fsplit-lto-unit"
+// NOUNIT-NOT: "-fsplit-lto-unit"
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: 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: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -793,6 +793,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
@@ -5108,6 +5108,12 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {
+CmdArgs.push_back("-fsplit-lto-unit");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fexperimental_isel,
options::OPT_fno_experimental_isel)) {
 CmdArgs.push_back("-mllvm");
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -207,6 +207,8 @@
   return Sanitizers.Mask & NeedsUnwindTables;
 }
 
+bool SanitizerArgs::needsLTO() const { return Sanitizers.Mask & NeedsLTO; }
+
 SanitizerArgs::SanitizerArgs(const ToolChain &TC,
  const llvm::opt::ArgList &Args) {
   SanitizerMask AllRemove = 0;  // During the loop below, the accumulated set of
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -808,7 +808,8 @@
   return;
   }
   PerModulePasses.add(createWriteThinLTOBitcodePass(
-  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr));
+  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr,
+  CodeGenOpts.

[PATCH] D53891: [LTO] Pass down LTOUnit codegen flag to bitcode writer

[Teresa Johnson via Phabricator via cfe-commits]

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

Update a couple tests due to new default of splitting off and new flag.


Repository:
  rC Clang

https://reviews.llvm.org/D53891

Files:
  include/clang/Driver/Options.td
  include/clang/Driver/SanitizerArgs.h
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  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,8 @@
+// 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=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=UNIT %s
+
+// UNIT: "-fsplit-lto-unit"
+// NOUNIT-NOT: "-fsplit-lto-unit"
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: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -793,6 +793,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
@@ -5108,6 +5108,12 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {
+CmdArgs.push_back("-fsplit-lto-unit");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fexperimental_isel,
options::OPT_fno_experimental_isel)) {
 CmdArgs.push_back("-mllvm");
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -207,6 +207,8 @@
   return Sanitizers.Mask & NeedsUnwindTables;
 }
 
+bool SanitizerArgs::needsLTO() const { return Sanitizers.Mask & NeedsLTO; }
+
 SanitizerArgs::SanitizerArgs(const ToolChain &TC,
  const llvm::opt::ArgList &Args) {
   SanitizerMask AllRemove = 0;  // During the loop below, the accumulated set of
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -808,7 +808,8 @@
   return;
   }
   PerModulePasses.add(createWriteThinLTOBitcodePass(
-  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr));
+  *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr,
+  CodeGenOpts.EnableSplitLTOUnit));
 } else {
   // Emit a module summary by default for Regular LTO except for ld64
  

[PATCH] D53891: [LTO] Pass down LTOUnit codegen flag to bitcode writer

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53891#1300789, @tejohnson wrote:

> - Switch the default of splitting lto units to off by default, unless 
> compiled with CFI or -fwhole-program-vtables.


Thinking ahead to when I add the index based WPD implementation, we are going 
to want to emit the type tests, etc for ThinLTO by default and keep splitting 
off. Would you prefer:

1. -flto=thin implies -fwhole-program-vtables, and have that latter option no 
longer implicitly enable splitting
2. -flto=thin directly enables the CodeGenOptions.WholeProgramVTables flag 
(which is only used by CGClass.cpp to insert the type tests/checked loads).
3. The code in CGClass.cpp that checks CodeGenOptions.WholeProgramVTables also 
checks CodeGenOptions.PrepareForThinLTO

(This assumes we want index-based WPD on by default for thinlto, which based on 
my measurements of minimal overhead should be fine and desirable.)


Repository:
  rC Clang

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] Pass down LTOUnit codegen flag to bitcode writer

[Teresa Johnson via Phabricator via cfe-commits]

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

Update to use new module flag


Repository:
  rC Clang

https://reviews.llvm.org/D53891

Files:
  include/clang/Driver/Options.td
  include/clang/Driver/SanitizerArgs.h
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  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,8 @@
+// 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=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=UNIT %s
+
+// UNIT: "-fsplit-lto-unit"
+// NOUNIT-NOT: "-fsplit-lto-unit"
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: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -793,6 +793,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
@@ -5108,6 +5108,12 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  bool EnableSplitLTOUnit = Args.hasFlag(
+  options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {
+CmdArgs.push_back("-fsplit-lto-unit");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fexperimental_isel,
options::OPT_fno_experimental_isel)) {
 CmdArgs.push_back("-mllvm");
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -207,6 +207,8 @@
   return Sanitizers.Mask & NeedsUnwindTables;
 }
 
+bool SanitizerArgs::needsLTO() const { return Sanitizers.Mask & NeedsLTO; }
+
 SanitizerArgs::SanitizerArgs(const ToolChain &TC,
  const llvm::opt::ArgList &Args) {
   SanitizerMask AllRemove = 0;  // During the loop below, the accumulated set of
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -807,6 +807,8 @@
 if (!ThinLinkOS)
   return;
   }
+  TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
+   CodeGenOpts.EnableSplitLTOUnit);
   PerModulePasses.add(createWriteThinLTOBitcodePass(
   *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr));
 } else {
@@ -817,12 +819,16 @@
!CodeGenOpts.DisableLLVMPasses &

[PATCH] D53524: [ThinLTO] Enable LTOUnit only when it is needed

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson abandoned this revision.
tejohnson added a comment.

Abandoned in favor of new approach in 
https://reviews.llvm.org/D53890/https://reviews.llvm.org/D53891.


Repository:
  rC Clang

https://reviews.llvm.org/D53524



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

[PATCH] D55048: [ThinLTO] Allow importing of multiple symbols with same GUID

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: evgeny777.
Herald added subscribers: dexonsmith, steven_wu, eraman, inglorion, mehdi_amini.

The is the clang side of the fix in D55047 , 
to handle the case where
two different modules have local variables with the same GUID because
they had the same source file name at compilation time. Allow multiple
symbols with the same GUID to be imported, and test that this case works
with the distributed backend path.

Depends on D55047 .


Repository:
  rC Clang

https://reviews.llvm.org/D55048

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
  test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
  test/CodeGen/thinlto_backend_local_name_conflict.ll

Index: test/CodeGen/thinlto_backend_local_name_conflict.ll
===
--- /dev/null
+++ test/CodeGen/thinlto_backend_local_name_conflict.ll
@@ -0,0 +1,34 @@
+; Test handling when two files with the same source file name contain
+; static read only variables with the same name (which will have the same GUID
+; in the combined index).
+
+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary -module-hash %s -o %t.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/thinlto_backend_local_name_conflict1.ll -o %t2.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/thinlto_backend_local_name_conflict2.ll -o %t3.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
+; RUN: llvm-lto -thinlto-action=distributedindexes -exported-symbol=main -thinlto-index=%t4.bc %t.bc
+
+; This module will import a() and b() which should cause the read only copy
+; of baz from each of those modules to be imported. Check that the both are
+; imported as local copies.
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t4.o -x ir %t.bc -c -fthinlto-index=%t.bc.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.3.import.bc -o - | FileCheck --check-prefix=IMPORT %s
+; IMPORT: @baz.llvm.{{.*}} = internal global i32 10
+; IMPORT: @baz.llvm.{{.*}} = internal global i32 10
+
+; ModuleID = 'local_name_conflict_var_main.o'
+source_filename = "local_name_conflict_var_main.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @main() {
+entry:
+  %call1 = call i32 (...) @a()
+  %call2 = call i32 (...) @b()
+  ret i32 0
+}
+
+declare i32 @a(...)
+declare i32 @b(...)
Index: test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
===
--- /dev/null
+++ test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
@@ -0,0 +1,13 @@
+; ModuleID = 'local_name_conflict_var.o'
+source_filename = "local_name_conflict_var.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@baz = internal global i32 10, align 4
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @b() {
+entry:
+  %0 = load i32, i32* @baz, align 4
+  ret i32 %0
+}
Index: test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
===
--- /dev/null
+++ test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
@@ -0,0 +1,13 @@
+; ModuleID = 'local_name_conflict_var.o'
+source_filename = "local_name_conflict_var.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@baz = internal global i32 10, align 4
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @a() {
+entry:
+  %0 = load i32, i32* @baz, align 4
+  ret i32 %0
+}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -1155,15 +1155,14 @@
   continue;
 
 auto GUID = GlobalList.first;
-assert(GlobalList.second.SummaryList.size() == 1 &&
-   "Expected individual combined index to have one summary per GUID");
-auto &Summary = GlobalList.second.SummaryList[0];
-// Skip the summaries for the importing module. These are included to
-// e.g. record required linkage changes.
-if (Summary->modulePath() == M->getModuleIdentifier())
-  continue;
-// Add an entry to provoke importing by thinBackend.
-ImportList[Summary->modulePath()].insert(GUID);
+for (auto &Summary : GlobalList.second.SummaryList) {
+  // Skip the summaries for the importing module. These are included to
+  // e.g. record required linkage changes.
+  if (Summary->modulePath() == M->getModuleIdentifier())
+continue;
+  // Add an entry to provoke importing by thinBackend.
+  ImportList[Summary->modulePath()].insert(GUID);
+}
   }
 
   std::vector> OwnedImports;
_

[PATCH] D55048: [ThinLTO] Allow importing of multiple symbols with same GUID

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347887: [ThinLTO] Allow importing of multiple symbols with 
same GUID (authored by tejohnson, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55048?vs=175814&id=175881#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55048

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
  test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
  test/CodeGen/thinlto_backend_local_name_conflict.ll

Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -1155,15 +1155,14 @@
   continue;
 
 auto GUID = GlobalList.first;
-assert(GlobalList.second.SummaryList.size() == 1 &&
-   "Expected individual combined index to have one summary per GUID");
-auto &Summary = GlobalList.second.SummaryList[0];
-// Skip the summaries for the importing module. These are included to
-// e.g. record required linkage changes.
-if (Summary->modulePath() == M->getModuleIdentifier())
-  continue;
-// Add an entry to provoke importing by thinBackend.
-ImportList[Summary->modulePath()].insert(GUID);
+for (auto &Summary : GlobalList.second.SummaryList) {
+  // Skip the summaries for the importing module. These are included to
+  // e.g. record required linkage changes.
+  if (Summary->modulePath() == M->getModuleIdentifier())
+continue;
+  // Add an entry to provoke importing by thinBackend.
+  ImportList[Summary->modulePath()].insert(GUID);
+}
   }
 
   std::vector> OwnedImports;
Index: test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
===
--- test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
+++ test/CodeGen/Inputs/thinlto_backend_local_name_conflict2.ll
@@ -0,0 +1,13 @@
+; ModuleID = 'local_name_conflict_var.o'
+source_filename = "local_name_conflict_var.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@baz = internal global i32 10, align 4
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @b() {
+entry:
+  %0 = load i32, i32* @baz, align 4
+  ret i32 %0
+}
Index: test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
===
--- test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
+++ test/CodeGen/Inputs/thinlto_backend_local_name_conflict1.ll
@@ -0,0 +1,13 @@
+; ModuleID = 'local_name_conflict_var.o'
+source_filename = "local_name_conflict_var.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@baz = internal global i32 10, align 4
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @a() {
+entry:
+  %0 = load i32, i32* @baz, align 4
+  ret i32 %0
+}
Index: test/CodeGen/thinlto_backend_local_name_conflict.ll
===
--- test/CodeGen/thinlto_backend_local_name_conflict.ll
+++ test/CodeGen/thinlto_backend_local_name_conflict.ll
@@ -0,0 +1,34 @@
+; Test handling when two files with the same source file name contain
+; static read only variables with the same name (which will have the same GUID
+; in the combined index).
+
+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary -module-hash %s -o %t.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/thinlto_backend_local_name_conflict1.ll -o %t2.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/thinlto_backend_local_name_conflict2.ll -o %t3.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
+; RUN: llvm-lto -thinlto-action=distributedindexes -exported-symbol=main -thinlto-index=%t4.bc %t.bc
+
+; This module will import a() and b() which should cause the read only copy
+; of baz from each of those modules to be imported. Check that the both are
+; imported as local copies.
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t4.o -x ir %t.bc -c -fthinlto-index=%t.bc.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.3.import.bc -o - | FileCheck --check-prefix=IMPORT %s
+; IMPORT: @baz.llvm.{{.*}} = internal global i32 10
+; IMPORT: @baz.llvm.{{.*}} = internal global i32 10
+
+; ModuleID = 'local_name_conflict_var_main.o'
+source_filename = "local_name_conflict_var_main.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @main() {
+entry:
+  %call1 = call i32 (...) @a()
+  %call2 = call i32 (...) @b()
+  ret i32 0
+}
+
+declare i32 @a(...)
+declare i32 @b(...)
___
cfe-commits mailing list
cfe

[PATCH] D59304: Fix invocation of Gold plugin with LTO after r355331

[Teresa Johnson via Phabricator via cfe-commits]

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

lgtm


Repository:
  rC Clang

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

https://reviews.llvm.org/D59304



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

[PATCH] D60163: [ThinLTO] Handle -fno-builtin* options for distributed backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: kristina, dexonsmith, steven_wu, eraman, inglorion, 
mehdi_amini.
Herald added a project: clang.

Without this patch, we were ignoring the -fno-builtin* options for clang
invocations performing ThinLTO distributed backends. Create and pass
down to LTO a TargetLibraryInfoImpl. A separate llvm-side patch will be
sent to add the new Config field and its necessary handling.

Depends on D60162 .


Repository:
  rC Clang

https://reviews.llvm.org/D60163

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/thinlto_backend_nobuiltin.ll


Index: test/CodeGen/thinlto_backend_nobuiltin.ll
===
--- /dev/null
+++ test/CodeGen/thinlto_backend_nobuiltin.ll
@@ -0,0 +1,30 @@
+; Make sure that -fno-builtin* is passed down and handled properly in
+; the ThinLTO distributed backend.
+; REQUIRES: x86-registered-target
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+
+; By default the optimizer will convert memset call to intinsic
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c 
-fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=BUILTIN
+
+; With -fno-builtin-memset it should not
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c 
-fthinlto-index=%t.thinlto.bc -save-temps=obj -fno-builtin-memset
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=NOBUILTIN
+
+; Similarly, with -fno-builtin it should not
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c 
-fthinlto-index=%t.thinlto.bc -save-temps=obj -fno-builtin
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=NOBUILTIN
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i8* @memset(i8*, i8, i64)
+
+define i8* @test(i8* %mem, i8 %val, i64 %size) {
+  ; BUILTIN: call void @llvm.memset.p0i8.i64
+  ; NOBUILTIN: call i8* @memset
+  %ret = call i8* @memset(i8* %mem, i8 %val, i64 %size)
+  ret i8* %ret
+}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -1327,6 +1327,9 @@
   Conf.CGOptLevel = getCGOptLevel(CGOpts);
   initTargetOptions(Conf.Options, CGOpts, TOpts, LOpts, HeaderOpts);
   Conf.SampleProfile = std::move(SampleProfile);
+  Triple TargetTriple(M->getTargetTriple());
+  std::unique_ptr TLII(createTLII(TargetTriple, 
CGOpts));
+  Conf.TLII = std::move(TLII);
 
   // Context sensitive profile.
   if (CGOpts.hasProfileCSIRInstr()) {


Index: test/CodeGen/thinlto_backend_nobuiltin.ll
===
--- /dev/null
+++ test/CodeGen/thinlto_backend_nobuiltin.ll
@@ -0,0 +1,30 @@
+; Make sure that -fno-builtin* is passed down and handled properly in
+; the ThinLTO distributed backend.
+; REQUIRES: x86-registered-target
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+
+; By default the optimizer will convert memset call to intinsic
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=BUILTIN
+
+; With -fno-builtin-memset it should not
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj -fno-builtin-memset
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=NOBUILTIN
+
+; Similarly, with -fno-builtin it should not
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj -fno-builtin
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=NOBUILTIN
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i8* @memset(i8*, i8, i64)
+
+define i8* @test(i8* %mem, i8 %val, i64 %size) {
+  ; BUILTIN: call void @llvm.memset.p0i8.i64
+  ; NOBUILTIN: call i8* @memset
+  %ret = call i8* @memset(i8* %mem, i8 %val, i64 %size)
+  ret i8* %ret
+}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -1327,6 +1327,9 @@
   Conf.CGOptLevel = getCGOptLevel(CGOpts);
   initTargetOptions(Conf.Options, CGOpts, TOpts, LOpts, HeaderOpts);
   Conf.SampleProfile = std::move(SampleProfile);
+  Triple TargetTriple(M->getTargetTriple());
+  std::unique_ptr TLII(createTLII(TargetTriple, CGOpts));
+  Conf.TLII = std::move(TLII);
 
   // Context sensitive profile.
   if (CGOpts.hasProfileCSIRInstr()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://list

[PATCH] D60516: [LTO] Add plumbing to save stats during LTO on Darwin.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

LGTM with a minor fix needed below.

Darwin still uses the old LTO API, which is why the lto Config based handling 
in the new LTO API (LTO.h/LTO.cpp) are not being used on this path.




Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:101
+"lto-stats-file",
+cl::desc("With PGO, include profile count in optimization remarks"),
+cl::Hidden);

Wrong description for this option


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60516



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

[PATCH] D60163: [ThinLTO] Handle -fno-builtin* options for distributed backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D60163



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

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: xur.
Herald added subscribers: dexonsmith, steven_wu, hiraditya, eraman, inglorion, 
mehdi_amini.
Herald added projects: clang, LLVM.

The opt level was not being passed down to the ThinLTO backend when
invoked via clang (for distributed ThinLTO).

This exposed an issue where the new PM was asserting if the Thin or
regular LTO backend pipelines were invoked with -O0 (not a new issue,
could be provoked by invoking in-process *LTO backends via linker using
new PM and -O0). Fix this similar to the old PM where -O0 only does the
necessary lowering of type metadata (WPD and LowerTypeTest passes) and
then quits, rather than asserting.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61022

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/thinlto-debug-pm.c
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/tools/gold/X86/opt-level.ll

Index: llvm/test/tools/gold/X86/opt-level.ll
===
--- llvm/test/tools/gold/X86/opt-level.ll
+++ llvm/test/tools/gold/X86/opt-level.ll
@@ -6,12 +6,25 @@
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext -plugin-opt=save-temps \
 ; RUN:-m elf_x86_64 \
 ; RUN:-plugin-opt=O1 -r -o %t.o %t.bc
-; RUN: llvm-dis < %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-O1 %s
+; RUN: llvm-dis < %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-O1 --check-prefix=CHECK-O1-OLDPM %s
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext -plugin-opt=save-temps \
 ; RUN:-m elf_x86_64 \
 ; RUN:-plugin-opt=O2 -r -o %t.o %t.bc
 ; RUN: llvm-dis < %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-O2 %s
 
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext -plugin-opt=save-temps \
+; RUN:-m elf_x86_64 --plugin-opt=new-pass-manager \
+; RUN:-plugin-opt=O0 -r -o %t.o %t.bc
+; RUN: llvm-dis < %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-O0 %s
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext -plugin-opt=save-temps \
+; RUN:-m elf_x86_64 --plugin-opt=new-pass-manager \
+; RUN:-plugin-opt=O1 -r -o %t.o %t.bc
+; RUN: llvm-dis < %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-O1 --check-prefix=CHECK-O1-NEWPM %s
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext -plugin-opt=save-temps \
+; RUN:-m elf_x86_64 --plugin-opt=new-pass-manager \
+; RUN:-plugin-opt=O2 -r -o %t.o %t.bc
+; RUN: llvm-dis < %t.o.0.4.opt.bc -o - | FileCheck --check-prefix=CHECK-O2 %s
+
 ; CHECK-O0: define internal void @foo(
 ; CHECK-O1: define internal void @foo(
 ; CHECK-O2-NOT: define internal void @foo(
@@ -36,7 +49,9 @@
 
 end:
   ; CHECK-O0: phi
-  ; CHECK-O1: select
+  ; CHECK-O1-OLDPM: select
+  ; The new PM does not do as many optimizations at O1
+  ; CHECK-O1-NEWPM: phi
   %r = phi i32 [ 1, %t ], [ 2, %f ]
   ret i32 %r
 }
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -1046,10 +1046,16 @@
 //
 // Also, WPD has access to more precise information than ICP and can
 // devirtualize more effectively, so it should operate on the IR first.
+//
+// The WPD and LowerTypeTest passes need to run at -O0 to lower type
+// metadata and intrinsics.
 MPM.addPass(WholeProgramDevirtPass(nullptr, ImportSummary));
 MPM.addPass(LowerTypeTestsPass(nullptr, ImportSummary));
   }
 
+  if (Level == O0)
+return MPM;
+
   // Force any function attributes we want the rest of the pipeline to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
@@ -1075,9 +1081,16 @@
 ModulePassManager
 PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level, bool DebugLogging,
  ModuleSummaryIndex *ExportSummary) {
-  assert(Level != O0 && "Must request optimizations for the default pipeline!");
   ModulePassManager MPM(DebugLogging);
 
+  if (Level == O0) {
+// The WPD and LowerTypeTest passes need to run at -O0 to lower type
+// metadata and intrinsics.
+MPM.addPass(WholeProgramDevirtPass(ExportSummary, nullptr));
+MPM.addPass(LowerTypeTestsPass(ExportSummary, nullptr));
+return MPM;
+  }
+
   if (PGOOpt && PGOOpt->Action == PGOOptions::SampleUse) {
 // Load sample profile before running the LTO optimization pipeline.
 MPM.addPass(SampleProfileLoaderPass(PGOOpt->ProfileFile,
Index: clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
===
--- clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
+++ clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
@@ -39,12 +39,12 @@
 ; CHECK-DIS: ^2 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi")

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.



Comment at: llvm/test/tools/gold/X86/opt-level.ll:53
+  ; CHECK-O1-OLDPM: select
+  ; The new PM does not do as many optimizations at O1
+  ; CHECK-O1-NEWPM: phi

mehdi_amini wrote:
> This is intended? I'm surprised the two PMs don't have the same list of 
> passes for a given opt level?
I'm really not sure. I did compare the post-link LTO pipelines of both PMs at 
O0/O1/O2 and confirmed that the old PM is doing many more passes than the new 
PM at O1. Probably a question for @chandlerc ? In any case, I didn't want to 
address it here since it is orthogonal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61022



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

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.



Comment at: llvm/test/tools/gold/X86/opt-level.ll:53
+  ; CHECK-O1-OLDPM: select
+  ; The new PM does not do as many optimizations at O1
+  ; CHECK-O1-NEWPM: phi

tejohnson wrote:
> mehdi_amini wrote:
> > This is intended? I'm surprised the two PMs don't have the same list of 
> > passes for a given opt level?
> I'm really not sure. I did compare the post-link LTO pipelines of both PMs at 
> O0/O1/O2 and confirmed that the old PM is doing many more passes than the new 
> PM at O1. Probably a question for @chandlerc ? In any case, I didn't want to 
> address it here since it is orthogonal.
Some more info:

This is the regular LTO post-link pipeline, ThinLTO post-link pipeline uses 
essentially the same as a normal opt pipeline so would be consistent at -O1.

The optimization missing from the new PM regular LTO post link pipeline that 
affects this test is SimplifyCFG. This and a few other optimizations are added 
in the old PM at O1 via PassManagerBuilder::addLateLTOOptimizationPasses. Note 
that PassManagerBuilder::addLTOOptimizationPasses does exit early at -O1 
(similar to where we exit early in the new PM for the regular LTO post link 
compilation). But the "late" LTO optimization passes are added unconditionally 
above -O0. Is this correct/desired? There isn't an equivalent in the new PM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61022



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

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.



Comment at: llvm/test/tools/gold/X86/opt-level.ll:53
+  ; CHECK-O1-OLDPM: select
+  ; The new PM does not do as many optimizations at O1
+  ; CHECK-O1-NEWPM: phi

chandlerc wrote:
> tejohnson wrote:
> > tejohnson wrote:
> > > mehdi_amini wrote:
> > > > This is intended? I'm surprised the two PMs don't have the same list of 
> > > > passes for a given opt level?
> > > I'm really not sure. I did compare the post-link LTO pipelines of both 
> > > PMs at O0/O1/O2 and confirmed that the old PM is doing many more passes 
> > > than the new PM at O1. Probably a question for @chandlerc ? In any case, 
> > > I didn't want to address it here since it is orthogonal.
> > Some more info:
> > 
> > This is the regular LTO post-link pipeline, ThinLTO post-link pipeline uses 
> > essentially the same as a normal opt pipeline so would be consistent at -O1.
> > 
> > The optimization missing from the new PM regular LTO post link pipeline 
> > that affects this test is SimplifyCFG. This and a few other optimizations 
> > are added in the old PM at O1 via 
> > PassManagerBuilder::addLateLTOOptimizationPasses. Note that 
> > PassManagerBuilder::addLTOOptimizationPasses does exit early at -O1 
> > (similar to where we exit early in the new PM for the regular LTO post link 
> > compilation). But the "late" LTO optimization passes are added 
> > unconditionally above -O0. Is this correct/desired? There isn't an 
> > equivalent in the new PM.
> I don't think it was intentional, but I'm not sure I would directly replicate 
> it if it requires adding an entirely new customization point. Is their some 
> simpler way of getting equivalent results at O1?
Yeah we can presumably just add those optimizations to the -O1 early exit path 
in PassBuilder::buildLTODefaultPipeline. I can send a patch (but would like to 
get this one in as it is a bugfix and orthogonal).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61022



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

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359025: [ThinLTO] Pass down opt level to LTO backend and 
handle -O0 LTO in new PM (authored by tejohnson, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61022?vs=196263&id=196296#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61022

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/test/CodeGen/thinlto-debug-pm.c
  cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  llvm/trunk/lib/Passes/PassBuilder.cpp
  llvm/trunk/test/tools/gold/X86/opt-level.ll

Index: cfe/trunk/test/CodeGen/thinlto-debug-pm.c
===
--- cfe/trunk/test/CodeGen/thinlto-debug-pm.c
+++ cfe/trunk/test/CodeGen/thinlto-debug-pm.c
@@ -1,10 +1,17 @@
-// Test to ensure -fdebug-pass-manager works when invoking the
-// ThinLTO backend path with the new PM.
+// Test to ensure the opt level is passed down to the ThinLTO backend.
 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -o %t.o -flto=thin -fexperimental-new-pass-manager -triple x86_64-unknown-linux-gnu -emit-llvm-bc %s
 // RUN: llvm-lto -thinlto -o %t %t.o
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -fexperimental-new-pass-manager 2>&1 | FileCheck %s
-// CHECK: Running pass:
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -fexperimental-new-pass-manager 2>&1 | FileCheck %s --check-prefix=O2-NEWPM
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O0 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -fexperimental-new-pass-manager 2>&1 | FileCheck %s --check-prefix=O0-NEWPM
+// O2-NEWPM: Running pass: LoopVectorizePass
+// O0-NEWPM-NOT: Running pass: LoopVectorizePass
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -debug-pass=Structure 2>&1 | FileCheck %s --check-prefix=O2-OLDPM
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O0 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -debug-pass=Structure 2>&1 | FileCheck %s --check-prefix=O0-OLDPM
+// O2-OLDPM: Loop Vectorization
+// O0-OLDPM-NOT: Loop Vectorization
 
 void foo() {
 }
Index: cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
===
--- cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
+++ cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
@@ -39,12 +39,12 @@
 ; CHECK-DIS: ^2 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi") ; guid = 7004155349499253778
 
 ; RUN: %clang_cc1 -triple x86_64-grtev4-linux-gnu \
-; RUN:   -emit-obj -fthinlto-index=%t.o.thinlto.bc \
+; RUN:   -emit-obj -fthinlto-index=%t.o.thinlto.bc -O2 \
 ; RUN:   -emit-llvm -o - -x ir %t.o | FileCheck %s --check-prefixes=CHECK-IR
 
 ; Check that backend does not fail generating native code.
 ; RUN: %clang_cc1 -triple x86_64-grtev4-linux-gnu \
-; RUN:   -emit-obj -fthinlto-index=%t.o.thinlto.bc \
+; RUN:   -emit-obj -fthinlto-index=%t.o.thinlto.bc -O2 \
 ; RUN:   -o %t.native.o -x ir %t.o
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -1325,6 +1325,7 @@
   Conf.MAttrs = TOpts.Features;
   Conf.RelocModel = CGOpts.RelocationModel;
   Conf.CGOptLevel = getCGOptLevel(CGOpts);
+  Conf.OptLevel = CGOpts.OptimizationLevel;
   initTargetOptions(Conf.Options, CGOpts, TOpts, LOpts, HeaderOpts);
   Conf.SampleProfile = std::move(SampleProfile);
 
Index: llvm/trunk/lib/Passes/PassBuilder.cpp
===
--- llvm/trunk/lib/Passes/PassBuilder.cpp
+++ llvm/trunk/lib/Passes/PassBuilder.cpp
@@ -1046,10 +1046,16 @@
 //
 // Also, WPD has access to more precise information than ICP and can
 // devirtualize more effectively, so it should operate on the IR first.
+//
+// The WPD and LowerTypeTest passes need to run at -O0 to lower type
+// metadata and intrinsics.
 MPM.addPass(WholeProgramDevirtPass(nullptr, ImportSummary));
 MPM.addPass(LowerTypeTestsPass(nullptr, ImportSummary));
   }
 
+  if (Level == O0)
+return MPM;
+
   // Force any function attributes we want the rest of the pipeline to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
@@ -1075,9 +1081,16 @@
 ModulePassManager
 PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level, bool DebugL

[PATCH] D52323: Add necessary support for storing code-model to module IR.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Note that if you add a line like:

"Depends on https://reviews.llvm.org/D52322"; in the summary that Phabricator 
will automatically link the two in the right way.




Comment at: lib/CodeGen/CodeGenModule.cpp:569
+  .Default(~0u);
+if ((CM != ~0u) && (CM != ~1u)) {
+  llvm::CodeModel::Model codeModel = 
static_cast(CM);

Can you simplify by using a single constant for both of these (since handling 
the same)?



Comment at: lib/CodeGen/CodeGenModule.h:38
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/Support/CodeGen.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"

Since nothing changed in this header, should this new include be moved to 
CodeGenModule.cpp?



Comment at: test/CodeGen/codemodels.c:2
+// RUN: %clang_cc1 -emit-llvm  %s -o - | FileCheck %s 
-check-prefix=CHECK-NOMODEL
+// RUN: %clang_cc1 -emit-llvm -mcode-model small %s -o - | FileCheck %s 
-check-prefix=CHECK-SMALL
+// RUN: %clang_cc1 -emit-llvm -mcode-model kernel %s -o - | FileCheck %s 
-check-prefix=CHECK-KERNEL

Might as well check "tiny" and "default" as well for completeness.


Repository:
  rC Clang

https://reviews.llvm.org/D52323



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

[PATCH] D52323: Add necessary support for storing code-model to module IR.

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D52323



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

[PATCH] D57265: [PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Overall it looks ok to me, but I'd like Chandler to comment regarding the 
preferred way to do this with the new PM, since we don't tend to use booleans 
there in the PassBuilder to control passes. Is it preferable to instead use a 
new function attribute instead of boolean flags on the PMs (e.g. the way 
-fno-inline is handled)?




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1331
+  Opts.SplitColdCode =
+  (Opts.OptimizationLevel > 0) && (Opts.OptimizeSize != 2) &&
+  Args.hasFlag(OPT_fsplit_cold_code, OPT_fno_split_cold_code, false);

would it be appropriate to give a warning when being ignored?


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

https://reviews.llvm.org/D57265



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

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.
Herald added a subscriber: jdoerfert.

LGTM except for place noted below where I disagree with a change made earlier. 
Will let @davidxl chime in if he disagrees with me or has any other comments.




Comment at: lib/CodeGen/BackendUtil.cpp:967
+  if (CodeGenOpts.hasProfileCSIRInstr()) {
+assert(!CodeGenOpts.hasProfileCSIRUse() &&
+   "Cannot have both CSProfileUse and CSProfileGen at the same time");

davidxl wrote:
> should emit error message instead.
I disagree with David's comment here. All the error checking should (and is) 
being done in Clang.cpp where the options are being processed. These and the 
places above for the old PM should just be asserts.


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

https://reviews.llvm.org/D54176



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

[PATCH] D37995: [Docs] Document cache pruning support for gold

[Teresa Johnson via Phabricator via cfe-commits]

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

LGTM with one fix noted.




Comment at: docs/ThinLTO.rst:144
 To help keep the size of the cache under control, ThinLTO supports cache
 pruning. Cache pruning is supported with ld64 and ELF and COFF lld, but
+currently only gold, ELF and COFF lld allow you to control the policy with a

add gold to this list too.


Repository:
  rL LLVM

https://reviews.llvm.org/D37995



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

aaronpuchert wrote:
> aaronpuchert wrote:
> > @pcc Your documentation for `DwoPath` suggests that this should be the 
> > actual output filename. However, the test that you added together with this 
> > line in rC333677 doesn't fail whatever garbage I write into that field 
> > here. What can I add to that so that it fails when we don't do the right 
> > thing here?
> @pcc Could you (or perhaps @tejohnson) comment on what the intended behavior 
> is here, and how I can change the test so that it fails when I do the wrong 
> thing? Is this the name of the file we write the split debug info to, or is 
> it the value we use for the DW_AT_[GNU_]dwo_name attribute in the skeleton CU?
It is the name of the file the split debug info is written to. If you test by 
changing the file name given to the -split-dwarf-file option in 
test/CodeGen/thinlto-split-dwarf.c, make sure you clean up the old one in your 
test output directory. When I tested it just now, it initially still passed 
because I had an old one sitting in the output directory from a prior run. Once 
I removed that it failed with the new name (without changing the corresponding 
llvm-readobj invocation).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

aaronpuchert wrote:
> tejohnson wrote:
> > aaronpuchert wrote:
> > > aaronpuchert wrote:
> > > > @pcc Your documentation for `DwoPath` suggests that this should be the 
> > > > actual output filename. However, the test that you added together with 
> > > > this line in rC333677 doesn't fail whatever garbage I write into that 
> > > > field here. What can I add to that so that it fails when we don't do 
> > > > the right thing here?
> > > @pcc Could you (or perhaps @tejohnson) comment on what the intended 
> > > behavior is here, and how I can change the test so that it fails when I 
> > > do the wrong thing? Is this the name of the file we write the split debug 
> > > info to, or is it the value we use for the DW_AT_[GNU_]dwo_name attribute 
> > > in the skeleton CU?
> > It is the name of the file the split debug info is written to. If you test 
> > by changing the file name given to the -split-dwarf-file option in 
> > test/CodeGen/thinlto-split-dwarf.c, make sure you clean up the old one in 
> > your test output directory. When I tested it just now, it initially still 
> > passed because I had an old one sitting in the output directory from a 
> > prior run. Once I removed that it failed with the new name (without 
> > changing the corresponding llvm-readobj invocation).
> Thanks, I didn't consider that. I wasn't even aware that test output is 
> persisted.
> 
> It seems `DwoPath` is used both as output filename and as value for 
> `DW_AT_[GNU_]dwo_name` in the skeleton CU. `llc` has separate options for 
> both: `-split-dwarf-file` for the attribute and `-split-dwarf-output` for the 
> output filename. I want to do the same for Clang. Then we should probably 
> separate them for LTO, too.
> 
> What is the use case for `-split-dwarf-file` with an individual thin backend? 
> Is that used for distributed/remote builds? Also is there any non-cc1 way to 
> use it? There is `--plugin-opt=dwo_dir=...` for the LTO linker plugin, but 
> that seems to go a different route.
This is the path taken for distributed ThinLTO, the plugin-opt passed to linker 
are for in-process ThinLTO. While -split-dwarf-file is a cc1 option, it is 
normally set automatically from the filename under -gsplit-dwarf (see where it 
is set in Clang::ConstructJob).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

lgtm for the LTO bits. Suggestion below for comment.




Comment at: llvm/include/llvm/LTO/Config.h:92
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU.
+  std::string SplitDwarfFile;

Probably needs a comment similar to the one below about being for running 
individual backends directly. Otherwise (for in process ThinLTO) we use the 
DwoDir.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: llvm/include/llvm/LTO/Config.h:92
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU.
+  std::string SplitDwarfFile;

aaronpuchert wrote:
> tejohnson wrote:
> > Probably needs a comment similar to the one below about being for running 
> > individual backends directly. Otherwise (for in process ThinLTO) we use the 
> > DwoDir.
> Sure. Any objections to the following wording?
> 
> ```
>   /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
>   /// attribute in the skeleton CU. This should generally only be used when
>   /// running an individual backend directly via thinBackend(), as otherwise
>   /// all objects would use the same .dwo file. Not used as output path.
> ```
Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673



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

[PATCH] D64458: add -fthinlto-index= option to clang-cl

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

LGTM, but please wait for @pcc to make sure he is satisfied.
I think it would be good to port the file type deduction to clang (possibly as 
a follow on), to make these consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64458



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

[PATCH] D64610: [clang] allow -fthinlto-index= without -x ir

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson accepted this revision.
tejohnson added a comment.

LGTM with the comment fix from @MaskRay


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64610



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1515176 , @tejohnson wrote:

> In D61634#1512020 , @gchatelet wrote:
>
> > AFAIU here is a coarse plan of what needs to happen
> >
> > 1. Add a `no-builtin` clang function attribute that has the same semantic 
> > as the `no-builtin` cmd line argument 
> > 
> > 2. Propagate it to the IR.
> >   - In the light of recent discussions and as @theraven suggested it seems 
> > more appropriate to encode them as individual IR attributes (e.g. 
> > `"no-builtin-memcpy"`, `"no-builtin-sqrt"`, etc..)
> > 3. Propagate/merge the `no-builtin` IR attribute for LTO by "updating 
> > `AttributeFuncs::areInlineCompatible` and/or 
> > `AttributeFuncs::mergeAttributesForInlining` and adding a new MergeRule in 
> > `include/llvm/IR/Attributes.td` and writing a function like 
> > `adjustCallerStackProbeSize`."
>
>
> This one isn't about LTO, but rather the inliner. You could have different 
> functions in the same module even without LTO that have incompatible 
> no-builtin attributes. There isn't any propagation required for LTO.
>
> > 4. Get inspiration from `TargetTransformInfo` to get `TargetLibraryInfo` on 
> > a per function basis for all passes and respect the IR attribute.
> > 
> >   I'm not familiar with 3 and 4 but I can definitely have a look. I'll 
> > update this patch to do 1 and 2 in the meantime. @tejohnson let me know how 
> > you want to proceed for your related patch 
> > . I'm happy to help if I can.
>
> I will mark that one obsolete. I can work on 4, it may just take some time to 
> get it all plumbed.


Checking in to see where we are on this issue. I haven't had any time to work 
on 4 but hopefully can start on that soon. But it needs the first part done to 
be effective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D65009: [LTO] Don't mark regular LTO units with EnableSplitLTOUnit

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: dexonsmith, steven_wu, inglorion, Prazek, mehdi_amini.
Herald added a project: clang.

LTO Unit splitting does not apply to regular LTO modules, only ThinLTO
(which must be consistently split into regular and Thin units for
optimizations such as whole program devirtualization and lower type
tests). Don't add the corresponding module flag to regular LTO modules,
to avoid mismatched module flag errors when linking a regular LTO module
with the regular LTO part of a split ThinLTO module.

This relies on a corresponding LTO patch that ignores the status of this
module flag on non-ThinLTO modules.

Depends on D65008 .


Repository:
  rC Clang

https://reviews.llvm.org/D65009

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/split-lto-unit.c


Index: test/CodeGen/split-lto-unit.c
===
--- /dev/null
+++ test/CodeGen/split-lto-unit.c
@@ -0,0 +1,13 @@
+// ; Check that -flto=thin without -fsplit-lto-unit has EnableSplitLTOUnit = 0
+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s
+// CHECK: !{i32 1, !"EnableSplitLTOUnit", i32 0}
+//
+// ; Check that -flto=thin with -fsplit-lto-unit has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto=thin -fsplit-lto-unit -emit-llvm-bc < %s | llvm-dis 
-o - | FileCheck %s --check-prefix=SPLIT
+// SPLIT: !{i32 1, !"EnableSplitLTOUnit", i32 1}
+//
+// ; Check that regular LTO has no EnableSplitLTOUnit flag
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | 
llvm-dis -o - | FileCheck %s --implicit-check-not="EnableSplitLTOUnit" 
--check-prefix=THINLTOOFF
+// THINLTOOFF: !{i32 1, !"ThinLTO", i32 0}
+
+int main() {}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -847,8 +847,6 @@
   if (EmitLTOSummary) {
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
-TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
- CodeGenOpts.EnableSplitLTOUnit);
   }
 
   PerModulePasses.add(createBitcodeWriterPass(


Index: test/CodeGen/split-lto-unit.c
===
--- /dev/null
+++ test/CodeGen/split-lto-unit.c
@@ -0,0 +1,13 @@
+// ; Check that -flto=thin without -fsplit-lto-unit has EnableSplitLTOUnit = 0
+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s
+// CHECK: !{i32 1, !"EnableSplitLTOUnit", i32 0}
+//
+// ; Check that -flto=thin with -fsplit-lto-unit has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto=thin -fsplit-lto-unit -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s --check-prefix=SPLIT
+// SPLIT: !{i32 1, !"EnableSplitLTOUnit", i32 1}
+//
+// ; Check that regular LTO has no EnableSplitLTOUnit flag
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s --implicit-check-not="EnableSplitLTOUnit" --check-prefix=THINLTOOFF
+// THINLTOOFF: !{i32 1, !"ThinLTO", i32 0}
+
+int main() {}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -847,8 +847,6 @@
   if (EmitLTOSummary) {
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
-TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
- CodeGenOpts.EnableSplitLTOUnit);
   }
 
   PerModulePasses.add(createBitcodeWriterPass(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65009: [LTO] Don't mark regular LTO units with EnableSplitLTOUnit

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D65009#1594009 , @pcc wrote:

> Sorry, just realized this. If I do
>
>   clang++ -c -flto a.cpp # "split"
>   clang++ -c -flto=thin b.cpp -fwhole-program-vtables # non-split
>   clang++ a.o b.o
>
>
> this should fail, right? If I'm not mistaken, this patch series will cause 
> this to succeed. I think we need to change this patch so that it always sets 
> `EnableSplitLTOUnit` to 1 for regular LTO, then you can drop the other patch.


This is a good point. We could detect and handle this during the LTO merging 
here, but I don't think it is worth it (it would essentially be treating 
regular LTO as split in any case). The change you suggested fixes the original 
bug, re running the llvm/clang tests now and will update the patches afterwards.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65009



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

[PATCH] D65009: [LTO] Always mark regular LTO units with EnableSplitLTOUnit=1

[Teresa Johnson via Phabricator via cfe-commits]

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

Address comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D65009

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/split-lto-unit.c


Index: test/CodeGen/split-lto-unit.c
===
--- /dev/null
+++ test/CodeGen/split-lto-unit.c
@@ -0,0 +1,12 @@
+// ; Check that -flto=thin without -fsplit-lto-unit has EnableSplitLTOUnit = 0
+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s
+// CHECK: !{i32 1, !"EnableSplitLTOUnit", i32 0}
+//
+// ; Check that -flto=thin with -fsplit-lto-unit has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto=thin -fsplit-lto-unit -emit-llvm-bc < %s | llvm-dis 
-o - | FileCheck %s --check-prefix=SPLIT
+// SPLIT: !{i32 1, !"EnableSplitLTOUnit", i32 1}
+//
+// ; Check that regular LTO has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | 
llvm-dis -o - | FileCheck %s --implicit-check-not="EnableSplitLTOUnit" 
--check-prefix=SPLIT
+
+int main() {}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -848,7 +848,7 @@
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
- CodeGenOpts.EnableSplitLTOUnit);
+ uint32_t(1));
   }
 
   PerModulePasses.add(createBitcodeWriterPass(


Index: test/CodeGen/split-lto-unit.c
===
--- /dev/null
+++ test/CodeGen/split-lto-unit.c
@@ -0,0 +1,12 @@
+// ; Check that -flto=thin without -fsplit-lto-unit has EnableSplitLTOUnit = 0
+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s
+// CHECK: !{i32 1, !"EnableSplitLTOUnit", i32 0}
+//
+// ; Check that -flto=thin with -fsplit-lto-unit has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto=thin -fsplit-lto-unit -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s --check-prefix=SPLIT
+// SPLIT: !{i32 1, !"EnableSplitLTOUnit", i32 1}
+//
+// ; Check that regular LTO has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s --implicit-check-not="EnableSplitLTOUnit" --check-prefix=SPLIT
+
+int main() {}
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -848,7 +848,7 @@
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
- CodeGenOpts.EnableSplitLTOUnit);
+ uint32_t(1));
   }
 
   PerModulePasses.add(createBitcodeWriterPass(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65009: [LTO] Always mark regular LTO units with EnableSplitLTOUnit=1

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366623: [LTO] Always mark regular LTO units with 
EnableSplitLTOUnit=1 (authored by tejohnson, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65009

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/test/CodeGen/split-lto-unit.c


Index: cfe/trunk/test/CodeGen/split-lto-unit.c
===
--- cfe/trunk/test/CodeGen/split-lto-unit.c
+++ cfe/trunk/test/CodeGen/split-lto-unit.c
@@ -0,0 +1,12 @@
+// ; Check that -flto=thin without -fsplit-lto-unit has EnableSplitLTOUnit = 0
+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s
+// CHECK: !{i32 1, !"EnableSplitLTOUnit", i32 0}
+//
+// ; Check that -flto=thin with -fsplit-lto-unit has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto=thin -fsplit-lto-unit -emit-llvm-bc < %s | llvm-dis 
-o - | FileCheck %s --check-prefix=SPLIT
+// SPLIT: !{i32 1, !"EnableSplitLTOUnit", i32 1}
+//
+// ; Check that regular LTO has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | 
llvm-dis -o - | FileCheck %s --implicit-check-not="EnableSplitLTOUnit" 
--check-prefix=SPLIT
+
+int main() {}
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -852,7 +852,7 @@
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
- CodeGenOpts.EnableSplitLTOUnit);
+ uint32_t(1));
   }
 
   PerModulePasses.add(createBitcodeWriterPass(


Index: cfe/trunk/test/CodeGen/split-lto-unit.c
===
--- cfe/trunk/test/CodeGen/split-lto-unit.c
+++ cfe/trunk/test/CodeGen/split-lto-unit.c
@@ -0,0 +1,12 @@
+// ; Check that -flto=thin without -fsplit-lto-unit has EnableSplitLTOUnit = 0
+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s
+// CHECK: !{i32 1, !"EnableSplitLTOUnit", i32 0}
+//
+// ; Check that -flto=thin with -fsplit-lto-unit has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto=thin -fsplit-lto-unit -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s --check-prefix=SPLIT
+// SPLIT: !{i32 1, !"EnableSplitLTOUnit", i32 1}
+//
+// ; Check that regular LTO has EnableSplitLTOUnit = 1
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | llvm-dis -o - | FileCheck %s --implicit-check-not="EnableSplitLTOUnit" --check-prefix=SPLIT
+
+int main() {}
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -852,7 +852,7 @@
 if (!TheModule->getModuleFlag("ThinLTO"))
   TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
- CodeGenOpts.EnableSplitLTOUnit);
+ uint32_t(1));
   }
 
   PerModulePasses.add(createBitcodeWriterPass(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.
Herald added a reviewer: jdoerfert.

I had some time to work on this finally late last week. I decided the most 
straightforward thing was to implement the necessary interface changes to the 
TLI analysis to make it require a Function (without any changes yet to how that 
analysis operates). See D66428  that I just 
mailed for review. That takes care of the most widespread changes needed for 
this migration, and afterwards we can change the analysis to look at the 
function attributes and make a truly per-function TLI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Adding Wei who works on SamplePGO on our end currently.




Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668
+// We must also remove exception handling before attaching sample profile
+// data.
+MPM.addPass(
+createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass()));

chandlerc wrote:
> Does the this need to go before the EarlyFPM as well as before the sample 
> profile loader?
> 
> Also, this is ... a pretty expensive thing to do... Do we really need this? 
> We've been using ThinLTO and sample PGO with the new PM for a long time and 
> haven't seen any real issue. I think we can probably just leave this 
> difference between the two pass managers and remove the test for this pass 
> running rather than adding it.
> 
> CC-ing some others just to double check, but I'm not aware of any issues 
> we've seen with PGO that were because of this discrepancy.
We don't tend to use exception handling so might not be hitting the issue 
described in the headline.

If this is fixing an issue with SamplePGO and EH cleanup interactions, can you 
add a test that tests for that (i.e. is there a code optimization issue 
currently?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: llvm-9.0.0-20190629/clang/lib/Driver/ToolChains/CommonArgs.cpp:395
+  if(OOpt == "s" || OOpt == "z")
+OOpt = "3";
+}

Os/Oz are closer to O2 than O3 (which allows much more aggressive code size 
increasing optimizations).

Better though to add a size level to the LTO::Config, teach lld to pass it 
through properly, then using the LTO Config to set the SizeLevel in the old PM 
and the PassBuilder::OptimizationLevel in the new PM when setting up the LTO 
backend pipelines, similar to how CodeGenLevel.OptimizeSize is handled in clang 
(BackendUtil.cpp).

My concern is that silently mapping Os/Oz to do something different than in the 
non-LTO pipeline is going to end up more confusing than the current error 
(which isn't good either, but at least makes it clear that it isn't supported).


Repository:
  rC Clang

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

https://reviews.llvm.org/D63976



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

[PATCH] D60162: [ThinLTO] Support TargetLibraryInfoImpl in the backend

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson updated this revision to Diff 199320.
tejohnson added a comment.
Herald added subscribers: cfe-commits, hiraditya, eraman.
Herald added a project: clang.

Rework using module flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/nobuiltins.c
  clang/test/CodeGen/svml-calls.ll
  clang/test/CodeGen/thinlto_backend_nobuiltin.ll
  clang/test/CodeGen/thinlto_backend_nobuiltin_memset.ll
  clang/test/CodeGen/veclib-calls.ll
  clang/test/CodeGen/veclib.c
  llvm/lib/LTO/LTOBackend.cpp

Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -218,6 +218,34 @@
   // FIXME (davide): verify the output.
 }
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =
+  new TargetLibraryInfoImpl(Triple(TM->getTargetTriple()));
+  if (auto *MD = mdconst::extract_or_null(
+  Mod.getModuleFlag("DisableAllBuiltins"))) {
+if (MD->getZExtValue())
+  TLII->disableAllFunctions();
+  } else if (Metadata *Val = Mod.getModuleFlag("NoBuiltins")) {
+// Disable individual libc/libm calls in TargetLibraryInfo.
+LibFunc F;
+for (const MDOperand &FuncName : cast(Val)->operands())
+  if (TLII->getLibFunc(cast(*FuncName).getString(), F))
+TLII->setUnavailable(F);
+  }
+
+  if (MDString *VL =
+  dyn_cast_or_null(Mod.getModuleFlag("VectorLibrary"))) {
+if (VL->getString() == "Accelerate")
+  TLII->addVectorizableFunctionsFromVecLib(
+  TargetLibraryInfoImpl::Accelerate);
+else if (VL->getString() == "SVML")
+  TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SVML);
+else
+  llvm_unreachable("Invalid vector library module flag");
+  }
+  return TLII;
+}
+
 static void runNewPMCustomPasses(Module &Mod, TargetMachine *TM,
  std::string PipelineDesc,
  std::string AAPipelineDesc,
@@ -239,6 +267,10 @@
   // Register the AA manager first so that our version is the one used.
   FAM.registerPass([&] { return std::move(AA); });
 
+  std::unique_ptr TLII(createTLII(Mod, TM));
+  FAM.registerPass([&] { return TargetLibraryAnalysis(*TLII); });
+  MAM.registerPass([&] { return TargetLibraryAnalysis(*TLII); });
+
   // Register all the basic analyses with the managers.
   PB.registerModuleAnalyses(MAM);
   PB.registerCGSCCAnalyses(CGAM);
@@ -268,7 +300,7 @@
   passes.add(createTargetTransformInfoWrapperPass(TM->getTargetIRAnalysis()));
 
   PassManagerBuilder PMB;
-  PMB.LibraryInfo = new TargetLibraryInfoImpl(Triple(TM->getTargetTriple()));
+  PMB.LibraryInfo = createTLII(Mod, TM);
   PMB.Inliner = createFunctionInliningPass();
   PMB.ExportSummary = ExportSummary;
   PMB.ImportSummary = ImportSummary;
Index: clang/test/CodeGen/veclib.c
===
--- /dev/null
+++ clang/test/CodeGen/veclib.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -emit-llvm -fveclib=Accelerate %s -o - | FileCheck %s -check-prefix=ACCELERATE
+// RUN: %clang_cc1 -emit-llvm -fveclib=SVML %s -o - | FileCheck %s -check-prefix=SVML
+// RUN: %clang_cc1 -emit-llvm -fveclib=none %s -o - | FileCheck %s -check-prefix=NONE
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s -check-prefix=NONE
+
+// ACCELERATE: !{{[0-9]+}} = !{i32 1, !"VectorLibrary", !"Accelerate"}
+// SVML: !{{[0-9]+}} = !{i32 1, !"VectorLibrary", !"SVML"}
+// NONE-NOT: VectorLibrary
+
+// Now ensure merging gets the expected behavior
+// RUN: %clang -c -flto %s -o %t0.o
+// RUN: %clang -c -flto -fveclib=Accelerate %s -o %t1.o
+// RUN: %clang -c -flto -fveclib=SVML %s -o %t2.o
+// Merge none with -fveclib=Accelerate -> VectorLibrary=Accelerate
+// RUN: llvm-lto %t0.o %t1.o -o %t3.o -save-merged-module
+// RUN: llvm-dis %t3.o.merged.bc -o - | FileCheck %s --check-prefix=ACCELERATE
+// Merge none with -fveclib=SVML -> VectorLibrary=SVML
+// RUN: llvm-lto %t0.o %t2.o -o %t3.o -save-merged-module
+// RUN: llvm-dis %t3.o.merged.bc -o - | FileCheck %s --check-prefix=SVML
+// Merge -fveclib=Accelerate with -fveclib=SVML -> Error
+// RUN: not llvm-lto %t1.o %t2.o -o %t3.o -save-merged-module
Index: clang/test/CodeGen/veclib-calls.ll
===
--- /dev/null
+++ clang/test/CodeGen/veclib-calls.ll
@@ -0,0 +1,38 @@
+; Test to ensure that -fveclib=Accelerate module flag is handled properly in
+; the ThinLTO distributed backend.
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+; RUN: %clang -target x86_64-unknown-linux-gnu -O3 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target t

[PATCH] D60162: [ThinLTO] Support TargetLibraryInfoImpl in the backend

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D60162#1498392 , @tejohnson wrote:

> In D60162#1498288 , @tejohnson wrote:
>
> > Working on this one again as it previously wasn't causing any issues but 
> > just now got exposed in multiple places and started causing correctness 
> > issues. Two questions:
> >
> > 1. What should be the behavior when merging modules e.g. for LTO. I'm 
> > thinking of the following, with multiple module flags to specify different 
> > aspects: a) For the NoBuiltinFuncs I'm thinking of a module flag containing 
> > the list of strings, with AppendUnique merging behavior, so we get a 
> > superset in the merged module b) For the flag to disable all builtins, have 
> > a separate module flag with a Max merge (so we get the most conservative 
> > behavior) c) For the VectorLibrary, not sure - should it be an Error to 
> > merge modules with different libraries?
> > 2. While we get the above hammered out, would it be ok to submit this one 
> > (and companion clang change) to fix the bug (then remove when the module 
> > flags are added)?
>
>
> Weird, phabricator added numbering to all of my bullets, which formatted very 
> differently than intended. Modified the above a bit to hopefully format 
> better (2 high level questions, and some subquestions on the first one).


I went ahead and implemented the above approach, PTAL. I added test cases for 
ensuring that the various clang driver flags set up the module flags properly, 
that merging behaves as expected, and that these options correctly affect LTO 
backends.

Note that this patch now includes both clang and llvm changes. I am going to 
mark the old clang child patch as obsolete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D60163: [ThinLTO] Handle -fno-builtin* options for distributed backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson abandoned this revision.
tejohnson added a comment.

The parent revision now includes both clang and llvm side changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60163



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1501066 , @gchatelet wrote:

> In D61634#1500453 , @efriedma wrote:
>
> > My main blocker is that I want to make sure we're moving in the right 
> > direction: towards LLVM IR with clear semantics, towards straightforward 
> > rules for writing freestanding C code, and towards solutions which behave 
> > appropriately for all targets.  There's clearly a problem here that's worth 
> > solving, but I want to make sure we're adding small features that interact 
> > with existing features in an obvious way.  The patch as written is adding a 
> > new attribute that changes the semantics of C and LLVM IR in a subtle way 
> > that interacts with existing optimizations and lowering in ways that are 
> > complex and hard to understand.
>
>
> This makes a lot of sense, I'm totally on board to reduce entropy and 
> confusion along the way.
>
> > I don't want to mix general restrictions on calling C library functions, 
> > with restrictions that apply specifically to memcpy: clang generally works 
> > on the assumption that a "memcpy" symbol is available in freestanding 
> > environments, and we don't really want to change that.
> > 
> > With -fno-builtin, specifically, we currently apply the restriction that 
> > optimizations will not introduce memcpy calls that would not exist with 
> > optimization disabled.  This is sort of artificial, and and maybe a bit 
> > confusing, but it seems to work well enough in practice.  gcc does 
> > something similar.
> > 
> > I don't really want to add an attribute that has a different meaning from 
> > -fno-builtin.  An attribute that has the same meaning is a lot less 
> > problematic: it reduces potential confusion, and solves a related problem 
> > that -fno-builtin currently doesn't really work with LTO, because we don't 
> > encode it into the IR.
>
> Adding @tejohnson to the conversation.
>
> IIUC you're offering to introduce something like 
> `__attribute__((no-builtin-memcpy))` or 
> `__attribute__((no-builtin("memcpy")))`.
>  As attributes they would compose nicely with (Thin)LTO.
>
> I believe we still want to turn `-fno-builtin` flags into attributes so 
> there's no impedance mismatch between the flag and the attribute right?


I have a related patch that turns -fno-builtin* options into module flags, and 
uses them in the LTO backends. This addresses current issues with these flags 
not working well with LTO.
See https://reviews.llvm.org/D60162


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Teresa Johnson via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/svml-calls.ll:16
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(

steven_wu wrote:
> Personally, I think codegen tests like this will be cleaner to keep in LLVM. 
> Clang tests just test the IRGen of the module flag and LLVM tests check that 
> those flags are respected and module flag merge is respected.
Ok. I originally was doing it here to ensure that ThinLTO distributed backends 
(which use clang) are fixed. But now that the approach no longer involves 
passing down additional info via that path into LTO, I don't think we need to 
test this explicitly but rather just as an LLVM LTO test. Will move.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

steven_wu wrote:
> Should this be done not just for LTOBackend but for regular compilation as 
> well? LegacyCodegenerator and llc can all be benefit from a matching 
> TargetLibraryInfo?
Yeah, probably. I think the best way to have this utilized everywhere is to 
move the below code into the TargetLibraryInfoImpl itself - by having it also 
take the Module as a parameter). Probably as a required parameter, to ensure it 
is used consistently. WDYT? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

tejohnson wrote:
> steven_wu wrote:
> > Should this be done not just for LTOBackend but for regular compilation as 
> > well? LegacyCodegenerator and llc can all be benefit from a matching 
> > TargetLibraryInfo?
> Yeah, probably. I think the best way to have this utilized everywhere is to 
> move the below code into the TargetLibraryInfoImpl itself - by having it also 
> take the Module as a parameter). Probably as a required parameter, to ensure 
> it is used consistently. WDYT? 
I meant, "into the TargetLibraryInfoImpl constructor"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1502138 , @hfinkel wrote:

> In D61634#1502043 , @efriedma wrote:
>
> > > I have a related patch that turns -fno-builtin* options into module flags
> >
> > Do you have any opinion on representing -fno-builtin using a module flag 
> > vs. a function attribute in IR?  It seems generally more flexible and 
> > easier to reason about a function attribute from my perspective.  But I 
> > might be missing something about the semantics of -fno-builtin that would 
> > make that representation awkward.  Or I guess it might just be more work to 
> > implement, given we have some IPO passes that use TargetLibraryInfo?
>
>
> I think that a function attribute would be better. We generally use these 
> flags only in the context of certain translation units, and when we use LTO, 
> it would be sad if we had to take the most-conservative settings across the 
> entire application. When we insert new function call to a standard library, 
> we always do it in the context of some function. We'd probably need to block 
> inlining in some cases, but that's better than a global conservative setting.


Using function level attributes instead of module flags does provide finer 
grained control and avoids the conservativeness when merging IR for LTO. The 
downsides I see, mostly just in terms of the engineering effort to get this to 
work, are:

- need to prevent inlining with different attributes
- currently the TargetLibraryInfo is constructed on a per-module basis. 
Presumably it would instead need to be created per Function - this one in 
particular seems like it would require fairly extensive changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1502685 , @gchatelet wrote:

> In D61634#1502201 , @tejohnson wrote:
>
> > Using function level attributes instead of module flags does provide finer 
> > grained control and avoids the conservativeness when merging IR for LTO. 
> > The downsides I see, mostly just in terms of the engineering effort to get 
> > this to work, are:
> >
> > - need to prevent inlining with different attributes
>
>
> IIUC this is needed regardless of the proposed change. Correct?


With the module flags approach, no - because there won't be fine grained enough 
info to do so. Any merged IR will need to use the conservatively set merged 
flag for the whole module. Or did you mean in comparison with the approach in 
this patch? I haven't looked at this one in any detail yet.

> 
> 
>> - currently the TargetLibraryInfo is constructed on a per-module basis. 
>> Presumably it would instead need to be created per Function - this one in 
>> particular seems like it would require fairly extensive changes.
> 
> Yes this one is a bit worrying.
>  I think we can discard right away any solution that would mutate or create a 
> TLI on a per function basis.
>  Another design would be something like the following:
> 
>   auto FunctionTLI = ModuleTLI.createCustomizedTLI(F);
> 
> 
> where `FunctionTLI` is itself a `TargetLibraryInfo` and calls to 
> `FunctionTLI` would either return the function customizations or delegate to 
> the module's TLI. WDYT?

I don't think this makes it much easier - all TLI users still need to be taught 
to get and use this new Function level TLI. I guess for Function (or lower) 
passes it would be fairly straightforward, but for things like Module level or 
SCC passes it will require more wiring to ensure that the FunctionTLI is 
accessed in the appropriate places. Doable just a lot of manual changes.

> I'm unsure if we want to support function level attribute right away or if 
> it's OK to be in an intermediate state with only module level attributes.

I'm interested in thoughts from other developers here. The module flag change 
is straightforward, but unnecessary churn if we want to go the function 
attribute route. Which despite the work seems like the best long term 
approach...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

There is discussion of using function attributes to control this instead, see 
https://reviews.llvm.org/D61634.
I'll hold off on making the planned changes here until the overall approach is 
decided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1512020 , @gchatelet wrote:

> AFAIU here is a coarse plan of what needs to happen
>
> 1. Add a `no-builtin` clang function attribute that has the same semantic as 
> the `no-builtin` cmd line argument 
> 
> 2. Propagate it to the IR.
>   - In the light of recent discussions and as @theraven suggested it seems 
> more appropriate to encode them as individual IR attributes (e.g. 
> `"no-builtin-memcpy"`, `"no-builtin-sqrt"`, etc..)
> 3. Propagate/merge the `no-builtin` IR attribute for LTO by "updating 
> `AttributeFuncs::areInlineCompatible` and/or 
> `AttributeFuncs::mergeAttributesForInlining` and adding a new MergeRule in 
> `include/llvm/IR/Attributes.td` and writing a function like 
> `adjustCallerStackProbeSize`."


This one isn't about LTO, but rather the inliner. You could have different 
functions in the same module even without LTO that have incompatible no-builtin 
attributes. There isn't any propagation required for LTO.

> 4. Get inspiration from `TargetTransformInfo` to get `TargetLibraryInfo` on a 
> per function basis for all passes and respect the IR attribute.
> 
>   I'm not familiar with 3 and 4 but I can definitely have a look. I'll update 
> this patch to do 1 and 2 in the meantime. @tejohnson let me know how you want 
> to proceed for your related patch . I'm 
> happy to help if I can.

I will mark that one obsolete. I can work on 4, it may just take some time to 
get it all plumbed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson abandoned this revision.
tejohnson added a comment.

As per D61634 , we are going to use function 
attributes instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D55620: [ThinLTO] Clang changes to utilize new pass to handle chains of aliases

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added reviewers: pcc, davidxl.
Herald added subscribers: dexonsmith, steven_wu, inglorion, mehdi_amini.

As with NameAnonGlobals, invoke the new CanonicalizeAliases via clang
when using the new PM.

Depends on D54507 .


Repository:
  rC Clang

https://reviews.llvm.org/D55620

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/lto-newpm-pipeline.c


Index: test/CodeGen/lto-newpm-pipeline.c
===
--- test/CodeGen/lto-newpm-pipeline.c
+++ test/CodeGen/lto-newpm-pipeline.c
@@ -27,12 +27,14 @@
 
 // CHECK-FULL-O0: Starting llvm::Module pass manager run.
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
+// CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 // CHECK-FULL-O0: Finished llvm::Module pass manager run.
 
 // CHECK-THIN-O0: Starting llvm::Module pass manager run.
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
+// CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 // CHECK-THIN-O0: Finished llvm::Module pass manager run.
@@ -48,6 +50,7 @@
 // LoopVectorizePass.
 // CHECK-THIN-OPTIMIZED: Starting llvm::Function pass manager run.
 // CHECK-THIN-OPTIMIZED-NOT: Running pass: LoopVectorizePass
+// CHECK-THIN-OPTIMIZED: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-OPTIMIZED: Running pass: NameAnonGlobalPass
 // CHECK-THIN-OPTIMIZED: Running pass: ThinLTOBitcodeWriterPass
 
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -59,6 +59,7 @@
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Utils.h"
+#include "llvm/Transforms/Utils/CanonicalizeAliases.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include 
@@ -996,8 +997,10 @@
 MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
   // Lastly, add a semantically necessary pass for LTO.
-  if (IsLTO || IsThinLTO)
+  if (IsLTO || IsThinLTO) {
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
+  }
 } else {
   // Map our optimization levels into one of the distinct levels used to
   // configure the pipeline.
@@ -1018,10 +1021,12 @@
   if (IsThinLTO) {
 MPM = PB.buildThinLTOPreLinkDefaultPipeline(
 Level, CodeGenOpts.DebugPassManager);
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
   } else if (IsLTO) {
 MPM = PB.buildLTOPreLinkDefaultPipeline(Level,
 CodeGenOpts.DebugPassManager);
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
   } else {
 MPM = PB.buildPerModuleDefaultPipeline(Level,


Index: test/CodeGen/lto-newpm-pipeline.c
===
--- test/CodeGen/lto-newpm-pipeline.c
+++ test/CodeGen/lto-newpm-pipeline.c
@@ -27,12 +27,14 @@
 
 // CHECK-FULL-O0: Starting llvm::Module pass manager run.
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
+// CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 // CHECK-FULL-O0: Finished llvm::Module pass manager run.
 
 // CHECK-THIN-O0: Starting llvm::Module pass manager run.
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
+// CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 // CHECK-THIN-O0: Finished llvm::Module pass manager run.
@@ -48,6 +50,7 @@
 // LoopVectorizePass.
 // CHECK-THIN-OPTIMIZED: Starting llvm::Function pass manager run.
 // CHECK-THIN-OPTIMIZED-NOT: Running pass: LoopVectorizePass
+// CHECK-THIN-OPTIMIZED: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-OPTIMIZED: Running pass: NameAnonGlobalPass
 // CHECK-THIN-OPTIMIZED: Running pass: ThinLTOBitcodeWriterPass
 
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -59,6 +59,7 @@
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Utils.h"
+#include "llvm/Transforms/Utils/CanonicalizeAliases.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include 
@@ -996,8 +997,10 @@
 MPM.addPass(createModuleToFunctionPassAdap

[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

[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 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] D55620: [ThinLTO] Clang changes to utilize new pass to handle chains of aliases

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

ping on the clang side change as well


Repository:
  rC Clang

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

https://reviews.llvm.org/D55620



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

[PATCH] D55620: [ThinLTO] Clang changes to utilize new pass to handle chains of aliases

[Teresa Johnson via Phabricator via cfe-commits]

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

Update comment


Repository:
  rC Clang

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

https://reviews.llvm.org/D55620

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/lto-newpm-pipeline.c


Index: test/CodeGen/lto-newpm-pipeline.c
===
--- test/CodeGen/lto-newpm-pipeline.c
+++ test/CodeGen/lto-newpm-pipeline.c
@@ -27,12 +27,14 @@
 
 // CHECK-FULL-O0: Starting llvm::Module pass manager run.
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
+// CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 // CHECK-FULL-O0: Finished llvm::Module pass manager run.
 
 // CHECK-THIN-O0: Starting llvm::Module pass manager run.
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
+// CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 // CHECK-THIN-O0: Finished llvm::Module pass manager run.
@@ -48,6 +50,7 @@
 // LoopVectorizePass.
 // CHECK-THIN-OPTIMIZED: Starting llvm::Function pass manager run.
 // CHECK-THIN-OPTIMIZED-NOT: Running pass: LoopVectorizePass
+// CHECK-THIN-OPTIMIZED: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-OPTIMIZED: Running pass: NameAnonGlobalPass
 // CHECK-THIN-OPTIMIZED: Running pass: ThinLTOBitcodeWriterPass
 
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -59,6 +59,7 @@
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Utils.h"
+#include "llvm/Transforms/Utils/CanonicalizeAliases.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include 
@@ -995,9 +996,11 @@
   if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))
 MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
-  // Lastly, add a semantically necessary pass for LTO.
-  if (IsLTO || IsThinLTO)
+  // Lastly, add semantically necessary passes for LTO.
+  if (IsLTO || IsThinLTO) {
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
+  }
 } else {
   // Map our optimization levels into one of the distinct levels used to
   // configure the pipeline.
@@ -1018,10 +1021,12 @@
   if (IsThinLTO) {
 MPM = PB.buildThinLTOPreLinkDefaultPipeline(
 Level, CodeGenOpts.DebugPassManager);
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
   } else if (IsLTO) {
 MPM = PB.buildLTOPreLinkDefaultPipeline(Level,
 CodeGenOpts.DebugPassManager);
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
   } else {
 MPM = PB.buildPerModuleDefaultPipeline(Level,


Index: test/CodeGen/lto-newpm-pipeline.c
===
--- test/CodeGen/lto-newpm-pipeline.c
+++ test/CodeGen/lto-newpm-pipeline.c
@@ -27,12 +27,14 @@
 
 // CHECK-FULL-O0: Starting llvm::Module pass manager run.
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
+// CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 // CHECK-FULL-O0: Finished llvm::Module pass manager run.
 
 // CHECK-THIN-O0: Starting llvm::Module pass manager run.
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
+// CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 // CHECK-THIN-O0: Finished llvm::Module pass manager run.
@@ -48,6 +50,7 @@
 // LoopVectorizePass.
 // CHECK-THIN-OPTIMIZED: Starting llvm::Function pass manager run.
 // CHECK-THIN-OPTIMIZED-NOT: Running pass: LoopVectorizePass
+// CHECK-THIN-OPTIMIZED: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-OPTIMIZED: Running pass: NameAnonGlobalPass
 // CHECK-THIN-OPTIMIZED: Running pass: ThinLTOBitcodeWriterPass
 
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -59,6 +59,7 @@
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Utils.h"
+#include "llvm/Transforms/Utils/CanonicalizeAliases.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include 
@@ -995,9 +996,11 @@
   if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))

[PATCH] D55620: [ThinLTO] Clang changes to utilize new pass to handle chains of aliases

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350424: [ThinLTO] Clang changes to utilize new pass to 
handle chains of aliases (authored by tejohnson, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55620

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/test/CodeGen/lto-newpm-pipeline.c


Index: cfe/trunk/test/CodeGen/lto-newpm-pipeline.c
===
--- cfe/trunk/test/CodeGen/lto-newpm-pipeline.c
+++ cfe/trunk/test/CodeGen/lto-newpm-pipeline.c
@@ -27,12 +27,14 @@
 
 // CHECK-FULL-O0: Starting llvm::Module pass manager run.
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
+// CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 // CHECK-FULL-O0: Finished llvm::Module pass manager run.
 
 // CHECK-THIN-O0: Starting llvm::Module pass manager run.
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
+// CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 // CHECK-THIN-O0: Finished llvm::Module pass manager run.
@@ -48,6 +50,7 @@
 // LoopVectorizePass.
 // CHECK-THIN-OPTIMIZED: Starting llvm::Function pass manager run.
 // CHECK-THIN-OPTIMIZED-NOT: Running pass: LoopVectorizePass
+// CHECK-THIN-OPTIMIZED: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-OPTIMIZED: Running pass: NameAnonGlobalPass
 // CHECK-THIN-OPTIMIZED: Running pass: ThinLTOBitcodeWriterPass
 
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -60,6 +60,7 @@
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
 #include "llvm/Transforms/Utils.h"
+#include "llvm/Transforms/Utils/CanonicalizeAliases.h"
 #include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include 
@@ -996,9 +997,11 @@
   if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))
 MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
-  // Lastly, add a semantically necessary pass for LTO.
-  if (IsLTO || IsThinLTO)
+  // Lastly, add semantically necessary passes for LTO.
+  if (IsLTO || IsThinLTO) {
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
+  }
 } else {
   // Map our optimization levels into one of the distinct levels used to
   // configure the pipeline.
@@ -1019,10 +1022,12 @@
   if (IsThinLTO) {
 MPM = PB.buildThinLTOPreLinkDefaultPipeline(
 Level, CodeGenOpts.DebugPassManager);
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
   } else if (IsLTO) {
 MPM = PB.buildLTOPreLinkDefaultPipeline(Level,
 CodeGenOpts.DebugPassManager);
+MPM.addPass(CanonicalizeAliasesPass());
 MPM.addPass(NameAnonGlobalPass());
   } else {
 MPM = PB.buildPerModuleDefaultPipeline(Level,


Index: cfe/trunk/test/CodeGen/lto-newpm-pipeline.c
===
--- cfe/trunk/test/CodeGen/lto-newpm-pipeline.c
+++ cfe/trunk/test/CodeGen/lto-newpm-pipeline.c
@@ -27,12 +27,14 @@
 
 // CHECK-FULL-O0: Starting llvm::Module pass manager run.
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
+// CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 // CHECK-FULL-O0: Finished llvm::Module pass manager run.
 
 // CHECK-THIN-O0: Starting llvm::Module pass manager run.
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
+// CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 // CHECK-THIN-O0: Finished llvm::Module pass manager run.
@@ -48,6 +50,7 @@
 // LoopVectorizePass.
 // CHECK-THIN-OPTIMIZED: Starting llvm::Function pass manager run.
 // CHECK-THIN-OPTIMIZED-NOT: Running pass: LoopVectorizePass
+// CHECK-THIN-OPTIMIZED: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-OPTIMIZED: Running pass: NameAnonGlobalPass
 // CHECK-THIN-OPTIMIZED: Running pass: ThinLTOBitcodeWriterPass
 
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -60,6 +60,7 @@
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scala

[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] D67385: Pass -mcmodel to LTO plugin

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Typically -mcmodel is passed to the clang compile invocation which sets a 
module flag in the IR, which is then used by LTO (see calls to 
Module::setCodeModel() and Module::getCodeModel()). Why is it necessary to pass 
through the mcmodel passed to the link invocation?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67385



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

[PATCH] D67385: Pass -mcmodel to LTO plugin

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D67385#1679127 , @khchen wrote:

> @tejohnson when I run the `clang -flto -Wl,-plugin-opt=-help` command, it 
> shows the
>
>   --code-model=   - Choose code model
>  =tiny-   Tiny code model
>  =small   -   Small code model
>  =kernel  -   Kernel code model
>  =medium  -   Medium code model
>  =large   -   Large code model
>
>   so I think the clang driver need to pass this info to LTO codegen, does it 
> make sense?


This is an internal LLVM option, that can be used to override the mcmodel set 
in the Module IR when the code was compiled into Bitcode. I don't think it 
makes sense to have the clang driver automatically set this internal option for 
the plugin, overridding what was set when the code was compiled, it can be set 
manually via a -plugin-opt if one needs to override for debugging etc. Why 
isn't the code model module flag in the Bitcode sufficient for your LTO case?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67385



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1635595 , @tejohnson wrote:

> I had some time to work on this finally late last week. I decided the most 
> straightforward thing was to implement the necessary interface changes to the 
> TLI analysis to make it require a Function (without any changes yet to how 
> that analysis operates). See D66428  that I 
> just mailed for review. That takes care of the most widespread changes needed 
> for this migration, and afterwards we can change the analysis to look at the 
> function attributes and make a truly per-function TLI.


D66428  went in a few weeks ago at r371284, 
and I just mailed the follow on patch D67923  
which will adds the support into the TLI analysis to use the Function to 
override the available builtins (with some of the code stubbed out since we 
don't yet have those per-Function attributes finalized).

@gchatelet where are you at on finalizing this patch? Also, I mentioned this 
offline but to follow up here: I think we will want an attribute to represent 
-fno-builtins (so that it doesn't need to be expanded out into the full list of 
individual no-builtin-{func} attributes, which would be both more verbose and 
less efficient, as well as being less backward compatible when new builtin 
funcs are added).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D67385: Pass -mcmodel to LTO plugin

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D67385#1680942 , @khchen wrote:

> @tejohnson for example:
>
>   $ clang -flto a.c -c -o a.o
>   $ llvm-ar q a.a a.o  // archive libraries  
>   $ clang -flto a.a b.c -O2 -o main -mcmodel=small 
>
>
> In above case user need to aware that passing 
> `-Wl,-plugin-opt=-code-model=small` to plugin is necessary.
>  It seems to me that is inconvenience because I believe user doesn't know it.


In general, -flto should be transparent to the build process. Think about what 
happens in your example if you remove -flto:

$ clang a.c -c -o a.o
$ llvm-ar q a.a a.o  // archive libraries  
$ clang a.a b.c -O2 -o main -mcmodel=small

a.o will be a native code built with the default mcmodel and -O0.

Generally, adding -flto shouldn't change this - I would argue that it would be 
unexpected to apply mcmodel=small as the user chose to compile a.c with the 
default mcmodel. We are going more and more toward a model where info is 
communicated via the IR from the compile step, as the mcmodel already is. Is 
there a compelling reason why a.c should not get the default mcmodel in this 
case (without having compiled it mcmodel=small in the first place)?

> I think this case is similar to if a user has a bitcode library compiled with 
> -O2 or -Oz, and link a program with -O3 -flto, what is expected behavior for 
> user?

There are a few things that are passed through the link command line to the 
plugin for legacy reasons, including -O2/-O3. However, note that in your above 
example the -O2 will not cause a.o to be O2 
 optimized: a.c is built at -O0 and 
this is encoded in the IR via function attributes (noinline and optnone), so 
the functions will not get O2 /O3 
 optimizations like inlining, etc.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67385



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

[PATCH] D68029: [ThinLTO] Enable index-only WPD from clang

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: pcc.
Herald added subscribers: arphaman, dexonsmith, steven_wu, inglorion, 
mehdi_amini.
Herald added a project: clang.

To trigger the index-only Whole Program Devirt support added to LLVM, we
need to be able to specify -fno-split-lto-unit in conjunction with
-fwhole-program-vtables. Keep the default for -fwhole-program-vtables as
-fsplit-lto-unit, but don't error on that option combination.


Repository:
  rC Clang

https://reviews.llvm.org/D68029

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/split-lto-unit.c


Index: test/Driver/split-lto-unit.c
===
--- test/Driver/split-lto-unit.c
+++ test/Driver/split-lto-unit.c
@@ -6,5 +6,5 @@
 
 // UNIT: "-fsplit-lto-unit"
 // NOUNIT-NOT: "-fsplit-lto-unit"
-// ERROR1: error: invalid argument '-fno-split-lto-unit' not allowed with 
'-fwhole-program-vtables'
+// ERROR1-NOT: error: invalid argument
 // ERROR2: error: invalid argument '-fno-split-lto-unit' not allowed with 
'-fsanitize=cfi'
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -5446,14 +5446,13 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
-  bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
+  bool DefaultsSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
   bool SplitLTOUnit =
   Args.hasFlag(options::OPT_fsplit_lto_unit,
-   options::OPT_fno_split_lto_unit, RequiresSplitLTOUnit);
-  if (RequiresSplitLTOUnit && !SplitLTOUnit)
-D.Diag(diag::err_drv_argument_not_allowed_with)
-<< "-fno-split-lto-unit"
-<< (WholeProgramVTables ? "-fwhole-program-vtables" : 
"-fsanitize=cfi");
+   options::OPT_fno_split_lto_unit, DefaultsSplitLTOUnit);
+  if (Sanitize.needsLTO() && !SplitLTOUnit)
+D.Diag(diag::err_drv_argument_not_allowed_with) << "-fno-split-lto-unit"
+<< "-fsanitize=cfi";
   if (SplitLTOUnit)
 CmdArgs.push_back("-fsplit-lto-unit");
 


Index: test/Driver/split-lto-unit.c
===
--- test/Driver/split-lto-unit.c
+++ test/Driver/split-lto-unit.c
@@ -6,5 +6,5 @@
 
 // UNIT: "-fsplit-lto-unit"
 // NOUNIT-NOT: "-fsplit-lto-unit"
-// ERROR1: error: invalid argument '-fno-split-lto-unit' not allowed with '-fwhole-program-vtables'
+// ERROR1-NOT: error: invalid argument
 // ERROR2: error: invalid argument '-fno-split-lto-unit' not allowed with '-fsanitize=cfi'
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -5446,14 +5446,13 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
-  bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
+  bool DefaultsSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
   bool SplitLTOUnit =
   Args.hasFlag(options::OPT_fsplit_lto_unit,
-   options::OPT_fno_split_lto_unit, RequiresSplitLTOUnit);
-  if (RequiresSplitLTOUnit && !SplitLTOUnit)
-D.Diag(diag::err_drv_argument_not_allowed_with)
-<< "-fno-split-lto-unit"
-<< (WholeProgramVTables ? "-fwhole-program-vtables" : "-fsanitize=cfi");
+   options::OPT_fno_split_lto_unit, DefaultsSplitLTOUnit);
+  if (Sanitize.needsLTO() && !SplitLTOUnit)
+D.Diag(diag::err_drv_argument_not_allowed_with) << "-fno-split-lto-unit"
+<< "-fsanitize=cfi";
   if (SplitLTOUnit)
 CmdArgs.push_back("-fsplit-lto-unit");
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68028: [clang] Add no_builtin attribute

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D68028#1685912 , @gchatelet wrote:

> @tejohnson I believe this is the missing part for D67923 
> .


Thanks, yep I will take a closer look at the patch today.

> I'm unsure if we still need the `BitVector` at all in the `TLI` since it 
> could be a simple attribute lookup on the function.

If we didn't save the info on the TLI we would instead need to save the 
Function object in the TLI and query the attribute info off the Function on 
every lookup, which seems heavier weight. I think caching the info in that 
object for fast lookup is going to be better. And as noted in the comments 
there, we can replace it with the existing AvailableArray moved from the base 
Impl object into the TLI and remove the override bitvector once this goes in, 
we use these attributes to set the TLI info on construction, and we remove the 
clang code that sets the unavailability from the CodeGenOpts which will no 
longer be needed. If this patch goes in first I can just modify my TLI patch to 
do that all in one go. Maybe that is best...

> Do you see any problematic interactions with the inlining phase?

The inliner will need to be modified to respect the function attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028



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

[PATCH] D67592: [Clang] Use -main-file-name for source filename if not set

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Please upload patches with context 
(https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

I'm not terribly familiar with this part of clang, but my concern would be that 
overriding the PresumedInputFile here could have unintended side effects. How 
about simply overriding the source file name on the Module object that 
eventually gets created, if it is "-"? I believe that is in the 
CodeGeneratorImpl constructor, and the CodeGenOptions are available there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67592



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

[PATCH] D67592: [Clang] Use -main-file-name for source filename if not set

[Teresa Johnson via Phabricator via cfe-commits]

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

lgtm


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

https://reviews.llvm.org/D67592



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

[PATCH] D67592: [Clang] Use -main-file-name for source filename if not set

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D67592#1686898 , @the_jk wrote:

> I don't have commit access so I'd need someone to push this, thanks.


I can commit for you today.


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

https://reviews.llvm.org/D67592



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

[PATCH] D67592: [Clang] Use -main-file-name for source filename if not set

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373217: [Clang] Use -main-file-name for source filename if 
not set (authored by tejohnson, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67592?vs=51&id=222437#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67592

Files:
  cfe/trunk/include/clang/Driver/CC1Options.td
  cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
  cfe/trunk/test/Frontend/stdin-input.c


Index: cfe/trunk/include/clang/Driver/CC1Options.td
===
--- cfe/trunk/include/clang/Driver/CC1Options.td
+++ cfe/trunk/include/clang/Driver/CC1Options.td
@@ -687,7 +687,7 @@
 def version : Flag<["-"], "version">,
   HelpText<"Print the compiler version">;
 def main_file_name : Separate<["-"], "main-file-name">,
-  HelpText<"Main file name to use for debug info">;
+  HelpText<"Main file name to use for debug info and source if missing">;
 def split_dwarf_output : Separate<["-"], "split-dwarf-output">,
   HelpText<"File name to use for split dwarf debug info output">;
 
Index: cfe/trunk/test/Frontend/stdin-input.c
===
--- cfe/trunk/test/Frontend/stdin-input.c
+++ cfe/trunk/test/Frontend/stdin-input.c
@@ -0,0 +1,7 @@
+// RUN: cat %s | %clang -emit-llvm -g -S \
+// RUN: -Xclang -main-file-name -Xclang test/foo.c -x c - -o - | FileCheck %s
+// CHECK: ; ModuleID = 'test/foo.c'
+// CHECK: source_filename = "test/foo.c"
+// CHECK: !1 = !DIFile(filename: "test/foo.c"
+
+int main() {}
Index: cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
===
--- cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
+++ cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
@@ -65,6 +65,13 @@
   private:
 SmallVector DeferredInlineMemberFuncDefs;
 
+static llvm::StringRef ExpandModuleName(llvm::StringRef ModuleName,
+const CodeGenOptions &CGO) {
+  if (ModuleName == "-" && !CGO.MainFileName.empty())
+return CGO.MainFileName;
+  return ModuleName;
+}
+
   public:
 CodeGeneratorImpl(DiagnosticsEngine &diags, llvm::StringRef ModuleName,
   const HeaderSearchOptions &HSO,
@@ -73,7 +80,8 @@
   CoverageSourceInfo *CoverageInfo = nullptr)
 : Diags(diags), Ctx(nullptr), HeaderSearchOpts(HSO),
   PreprocessorOpts(PPO), CodeGenOpts(CGO), HandlingTopLevelDecls(0),
-  CoverageInfo(CoverageInfo), M(new llvm::Module(ModuleName, C)) {
+  CoverageInfo(CoverageInfo),
+  M(new llvm::Module(ExpandModuleName(ModuleName, CGO), C)) {
   C.setDiscardValueNames(CGO.DiscardValueNames);
 }
 
@@ -121,7 +129,7 @@
 llvm::Module *StartModule(llvm::StringRef ModuleName,
   llvm::LLVMContext &C) {
   assert(!M && "Replacing existing Module?");
-  M.reset(new llvm::Module(ModuleName, C));
+  M.reset(new llvm::Module(ExpandModuleName(ModuleName, CodeGenOpts), C));
   Initialize(*Ctx);
   return M.get();
 }


Index: cfe/trunk/include/clang/Driver/CC1Options.td
===
--- cfe/trunk/include/clang/Driver/CC1Options.td
+++ cfe/trunk/include/clang/Driver/CC1Options.td
@@ -687,7 +687,7 @@
 def version : Flag<["-"], "version">,
   HelpText<"Print the compiler version">;
 def main_file_name : Separate<["-"], "main-file-name">,
-  HelpText<"Main file name to use for debug info">;
+  HelpText<"Main file name to use for debug info and source if missing">;
 def split_dwarf_output : Separate<["-"], "split-dwarf-output">,
   HelpText<"File name to use for split dwarf debug info output">;
 
Index: cfe/trunk/test/Frontend/stdin-input.c
===
--- cfe/trunk/test/Frontend/stdin-input.c
+++ cfe/trunk/test/Frontend/stdin-input.c
@@ -0,0 +1,7 @@
+// RUN: cat %s | %clang -emit-llvm -g -S \
+// RUN: -Xclang -main-file-name -Xclang test/foo.c -x c - -o - | FileCheck %s
+// CHECK: ; ModuleID = 'test/foo.c'
+// CHECK: source_filename = "test/foo.c"
+// CHECK: !1 = !DIFile(filename: "test/foo.c"
+
+int main() {}
Index: cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
===
--- cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
+++ cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
@@ -65,6 +65,13 @@
   private:
 SmallVector DeferredInlineMemberFuncDefs;
 
+static llvm::StringRef ExpandModuleName(llvm::StringRef ModuleName,
+const CodeGenOptions &CGO) {
+  if (ModuleName == "-" && !CGO.MainFileName.empty())
+return CGO.MainFileName;
+  return ModuleName;
+}
+
   public:
 CodeGeneratorImpl(Diagnost

[PATCH] D67592: [Clang] Use -main-file-name for source filename if not set

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D67592#1688413 , @thakis wrote:

> This is failing on my mac like so:


This should be fixed by r373237. I didn't get any email with buildbot failures, 
but someone mailed one to me manually. Not sure why it only fails sometimes.

> 
> 
>   FAIL: Clang :: Frontend/stdin-input.c (1 of 1)
>    TEST 'Clang :: Frontend/stdin-input.c' FAILED 
> 
>   Script:
>   --
>   : 'RUN: at line 1';   cat 
> /Users/thakis/src/llvm-project/clang/test/Frontend/stdin-input.c | 
> /Users/thakis/src/llvm-project/out/gn/bin/clang -emit-llvm -g -S  -Xclang 
> -main-file-name -Xclang test/foo.c -x c - -o - | 
> /Users/thakis/src/llvm-project/out/gn/bin/FileCheck 
> /Users/thakis/src/llvm-project/clang/test/Frontend/stdin-input.c
>   --
>   Exit Code: 1
>   
>   Command Output (stderr):
>   --
>   + : 'RUN: at line 1'
>   + cat /Users/thakis/src/llvm-project/clang/test/Frontend/stdin-input.c
>   + /Users/thakis/src/llvm-project/out/gn/bin/clang -emit-llvm -g -S -Xclang 
> -main-file-name -Xclang test/foo.c -x c - -o -
>   + /Users/thakis/src/llvm-project/out/gn/bin/FileCheck 
> /Users/thakis/src/llvm-project/clang/test/Frontend/stdin-input.c
>   /Users/thakis/src/llvm-project/clang/test/Frontend/stdin-input.c:5:11: 
> error: CHECK: expected string not found in input
>   // CHECK: !1 = !DIFile(filename: "test/foo.c"
> ^
>   :3:1: note: scanning from here
>   target datalayout = 
> "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
>   ^
>   :24:1: note: possible intended match here
>   !6 = !DIFile(filename: "test/foo.c", directory: 
> "/Users/thakis/src/llvm-project/out/gn/obj/clang/test/Frontend")
>   ^
>   
>   --
>   
>   
>   Testing Time: 0.06s
>   
>   Failing Tests (1):
>   Clang :: Frontend/stdin-input.c
> 
> 
> If I manually run `bin/llvm-lit clang/test/Frontend/stdin-input.c` it fails, 
> but if I run the command it claims to be running it passes.
> 
> If I replace the `FileCheck` on the run line with `> tmp.txt` I see what the 
> failing FileCheck is seeing:
> 
>   !0 = !{i32 2, !"SDK Version", [2 x i32] [i32 10, i32 14]}
>   !1 = !{i32 2, !"Dwarf Version", i32 4}
>   !2 = !{i32 2, !"Debug Info Version", i32 3}
>   !3 = !{i32 1, !"wchar_size", i32 4}
>   !4 = !{i32 7, !"PIC Level", i32 2}
>   !5 = distinct !DICompileUnit(language: DW_LANG_C99, file: !6, producer: 
> "clang version 10.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: 
> FullDebug, enums: !7, nameTableKind: None)
>   !6 = !DIFile(filename: "test/foo.c", directory: 
> "/Users/thakis/src/llvm-project/out/gn/obj/clang/test/Frontend")
> 
> 
> I'm guessing lit sets some env var that makes clang emit these additional 
> values (?)




Repository:
  rL LLVM

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

https://reviews.llvm.org/D67592



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

[PATCH] D68029: [ThinLTO] Enable index-only WPD from clang

[Teresa Johnson via Phabricator via cfe-commits]

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

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D68029



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

[PATCH] D68029: [ThinLTO] Enable index-only WPD from clang

[Teresa Johnson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373370: [ThinLTO] Enable index-only WPD from clang (authored 
by tejohnson, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D68029

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/split-lto-unit.c


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
@@ -6,5 +6,5 @@
 
 // UNIT: "-fsplit-lto-unit"
 // NOUNIT-NOT: "-fsplit-lto-unit"
-// ERROR1: error: invalid argument '-fno-split-lto-unit' not allowed with 
'-fwhole-program-vtables'
+// ERROR1-NOT: error: invalid argument
 // ERROR2: error: invalid argument '-fno-split-lto-unit' not allowed with 
'-fsanitize=cfi'
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -5456,14 +5456,13 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
-  bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
+  bool DefaultsSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
   bool SplitLTOUnit =
   Args.hasFlag(options::OPT_fsplit_lto_unit,
-   options::OPT_fno_split_lto_unit, RequiresSplitLTOUnit);
-  if (RequiresSplitLTOUnit && !SplitLTOUnit)
-D.Diag(diag::err_drv_argument_not_allowed_with)
-<< "-fno-split-lto-unit"
-<< (WholeProgramVTables ? "-fwhole-program-vtables" : 
"-fsanitize=cfi");
+   options::OPT_fno_split_lto_unit, DefaultsSplitLTOUnit);
+  if (Sanitize.needsLTO() && !SplitLTOUnit)
+D.Diag(diag::err_drv_argument_not_allowed_with) << "-fno-split-lto-unit"
+<< "-fsanitize=cfi";
   if (SplitLTOUnit)
 CmdArgs.push_back("-fsplit-lto-unit");
 


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
@@ -6,5 +6,5 @@
 
 // UNIT: "-fsplit-lto-unit"
 // NOUNIT-NOT: "-fsplit-lto-unit"
-// ERROR1: error: invalid argument '-fno-split-lto-unit' not allowed with '-fwhole-program-vtables'
+// ERROR1-NOT: error: invalid argument
 // ERROR2: error: invalid argument '-fno-split-lto-unit' not allowed with '-fsanitize=cfi'
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -5456,14 +5456,13 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
-  bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
+  bool DefaultsSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
   bool SplitLTOUnit =
   Args.hasFlag(options::OPT_fsplit_lto_unit,
-   options::OPT_fno_split_lto_unit, RequiresSplitLTOUnit);
-  if (RequiresSplitLTOUnit && !SplitLTOUnit)
-D.Diag(diag::err_drv_argument_not_allowed_with)
-<< "-fno-split-lto-unit"
-<< (WholeProgramVTables ? "-fwhole-program-vtables" : "-fsanitize=cfi");
+   options::OPT_fno_split_lto_unit, DefaultsSplitLTOUnit);
+  if (Sanitize.needsLTO() && !SplitLTOUnit)
+D.Diag(diag::err_drv_argument_not_allowed_with) << "-fno-split-lto-unit"
+<< "-fsanitize=cfi";
   if (SplitLTOUnit)
 CmdArgs.push_back("-fsplit-lto-unit");
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

ping


Repository:
  rC Clang

https://reviews.llvm.org/D45217



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

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

[Teresa Johnson via Phabricator via cfe-commits]

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



Comment at: include/clang/Driver/Options.td:2255
   HelpText<"Save intermediate compilation results.">;
-def save_temps : Flag<["-", "--"], "save-temps">, Flags<[DriverOption]>,
+def save_temps : Flag<["-", "--"], "save-temps">, Flags<[CC1Option, 
DriverOption]>,
   Alias, AliasArgs<["cwd"]>,

pcc wrote:
> This is still just a DriverOption, right?
No, we need to pass to cc1 to get it into the CodeGenOptions. See the changes 
to Clang.cpp (passes to cc1 invocation) and CompilerInvocation.cpp (consumes 
during cc1 invocation).



Comment at: lib/Frontend/CompilerInvocation.cpp:747
+.Case("obj", FrontendOpts.OutputFile)
+.Default("./" +
+ llvm::sys::path::filename(FrontendOpts.OutputFile).str());

pcc wrote:
> Why do you need the `"./"` part?
Not needed, removed.



Comment at: test/CodeGen/thinlto_backend.ll:30
 ; Ensure f2 was imported
-; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c 
-fthinlto-index=%t.thinlto.bc
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c 
-fthinlto-index=%t.thinlto.bc -save-temps=obj
 ; RUN: llvm-nm %t3.o | FileCheck --check-prefix=CHECK-OBJ %s

pcc wrote:
> Should there be another test for `-save-temps=cwd`?
Added -save-temps=cwd and -save-temps


Repository:
  rC Clang

https://reviews.llvm.org/D45217



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