[PATCH] D74490: [Clang] Limit -fintegrated-cc1 to only one TU

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Cherry-picked to 10.x as 9c9e46d786d0581079e5018e550cbe187a1ec219 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74490



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

[PATCH] D74490: [Clang] Limit -fintegrated-cc1 to only one TU

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG20f1abe306d0: [Clang] Limit -fintegrated-cc1 to only one TU 
(authored by aganea).

Changed prior to commit:
  https://reviews.llvm.org/D74490?vs=244197=244276#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74490

Files:
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cc1-spawnprocess.c

Index: clang/test/Driver/cc1-spawnprocess.c
===
--- clang/test/Driver/cc1-spawnprocess.c
+++ clang/test/Driver/cc1-spawnprocess.c
@@ -1,22 +1,37 @@
-// RUN: %clang -fintegrated-cc1 -### %s 2>&1 | FileCheck %s --check-prefix=YES
-// RUN: %clang -fno-integrated-cc1 -### %s 2>&1 | FileCheck %s --check-prefix=NO
+// RUN: %clang -fintegrated-cc1 -c -### %s 2>&1 | FileCheck %s --check-prefix=YES
+// RUN: %clang -fno-integrated-cc1 -c -### %s 2>&1 | FileCheck %s --check-prefix=NO
 
-// RUN: %clang -fintegrated-cc1 -fno-integrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fintegrated-cc1 -fno-integrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=NO
-// RUN: %clang -fno-integrated-cc1 -fintegrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fno-integrated-cc1 -fintegrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES
 
-// RUN: %clang_cl -fintegrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fintegrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES
-// RUN: %clang_cl -fno-integrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fno-integrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=NO
 
 // RUN: env CCC_OVERRIDE_OPTIONS=+-fintegrated-cc1 \
-// RUN: %clang -fintegrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fintegrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES
 // RUN: env CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1 \
-// RUN: %clang -fintegrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fintegrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=NO
 
 // YES: (in-process)
 // NO-NOT: (in-process)
+
+// The following tests ensure that only one integrated-cc1 is executed.
+
+// Only one TU, one job, thus integrated-cc1 is enabled.
+// RUN: %clang -fintegrated-cc1 -c %s -### 2>&1 | FileCheck %s --check-prefix=YES
+
+// Only one TU, but we're linking, two jobs, thus integrated-cc1 is disabled.
+// RUN: %clang -fintegrated-cc1 %s -### 2>&1 | FileCheck %s --check-prefix=NO
+
+// RUN: echo 'int main() { return f() + g(); }' > %t1.cpp
+// RUN: echo 'int f() { return 1; }' > %t2.cpp
+// RUN: echo 'int g() { return 2; }' > %t3.cpp
+
+// Three jobs, thus integrated-cc1 is disabled.
+// RUN: %clang -fintegrated-cc1 -c %t1.cpp %t2.cpp %t3.cpp -### 2>&1 | FileCheck %s --check-prefix=NO
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -6146,7 +6146,7 @@
   if (Output.getType() == types::TY_Object &&
   Args.hasFlag(options::OPT__SLASH_showFilenames,
options::OPT__SLASH_showFilenames_, false)) {
-C.getJobs().getJobs().back()->setPrintInputFilenames(true);
+C.getJobs().getJobs().back()->PrintInputFilenames = true;
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
Index: clang/lib/Driver/Job.cpp
===
--- clang/lib/Driver/Job.cpp
+++ clang/lib/Driver/Job.cpp
@@ -371,14 +371,29 @@
/*memoryLimit*/ 0, ErrMsg, ExecutionFailed);
 }
 
+CC1Command::CC1Command(const Action , const Tool ,
+   const char *Executable,
+   const llvm::opt::ArgStringList ,
+   ArrayRef Inputs)
+: Command(Source, Creator, Executable, Arguments, Inputs) {
+  InProcess = true;
+}
+
 void CC1Command::Print(raw_ostream , const char *Terminator, bool Quote,
CrashReportInfo *CrashInfo) const {
-  OS << " (in-process)\n";
+  if (InProcess)
+OS << " (in-process)\n";
   Command::Print(OS, Terminator, Quote, CrashInfo);
 }
 
-int CC1Command::Execute(ArrayRef> /*Redirects*/,
+int CC1Command::Execute(ArrayRef> Redirects,
 std::string *ErrMsg, bool *ExecutionFailed) const {
+  // FIXME: Currently, if there're more than one job, we disable
+  // -fintegrate-cc1. If we're no longer a integrated-cc1 job, fallback to
+  // out-of-process execution. See discussion in https://reviews.llvm.org/D74447
+  if (!InProcess)
+return Command::Execute(Redirects, ErrMsg, ExecutionFailed);
+
   PrintFileNames();
 
   SmallVector Argv;
Index: clang/lib/Driver/Driver.cpp
===

[PATCH] D74490: [Clang] Limit -fintegrated-cc1 to only one TU

[Alexandre Ganea via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Driver/Job.h:87
+  /// Whether to print the input filenames when executing.
+  bool PrintInputFilenames;
+

I forgot to put back the default value(s) here, I'll do it before landing.



Comment at: clang/tools/driver/cc1_main.cpp:262
   llvm::timeTraceProfilerCleanup();
+  Clang->clearOutputFiles(false);
 }

I'll commit this separately, this is not a requirement for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74490



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

[PATCH] D74490: [Clang] Limit -fintegrated-cc1 to only one TU

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm




Comment at: clang/test/Driver/cc1-spawnprocess.c:9
 
-// RUN: %clang_cl -fintegrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fintegrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES

aganea wrote:
> Out of curiosity: what does `--` means to do? (here used along with %s)
It indicates that the remaining args should be interpreted as inputs, not 
flags. We need `--` in the test suite on Mac where %s usually expands to 
`/Users/foo/bar.c`, which looks like `clang-cl /UMYMACRO`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74490



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

[PATCH] D74490: [Clang] Limit -fintegrated-cc1 to only one TU

[Alexandre Ganea via Phabricator via cfe-commits]

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



Comment at: clang/test/Driver/cc1-spawnprocess.c:9
 
-// RUN: %clang_cl -fintegrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fintegrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES

Out of curiosity: what does `--` means to do? (here used along with %s)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74490



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

[PATCH] D74490: [Clang] Limit -fintegrated-cc1 to only one TU

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: thakis, rnk, hans, erichkeane.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
aganea marked an inline comment as done.
aganea added inline comments.



Comment at: clang/test/Driver/cc1-spawnprocess.c:9
 
-// RUN: %clang_cl -fintegrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fintegrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES

Out of curiosity: what does `--` means to do? (here used along with %s)


As discussed in D74447 , this patch disables 
integrated-cc1 behavior if there's more than one job to be executed. This is 
meant to limit memory bloating, given that currently jobs don't clean up after 
execution (`-disable-free` is always active in cc1 mode).

I see this behavior as temporary until release 10.0 ships, then we'll 
reevaluate the situation, see if D74447  makes 
more sense on the long term.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74490

Files:
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cc1-spawnprocess.c
  clang/tools/driver/cc1_main.cpp

Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -259,6 +259,7 @@
   // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
   profilerOutput->flush();
   llvm::timeTraceProfilerCleanup();
+  Clang->clearOutputFiles(false);
 }
   }
 
Index: clang/test/Driver/cc1-spawnprocess.c
===
--- clang/test/Driver/cc1-spawnprocess.c
+++ clang/test/Driver/cc1-spawnprocess.c
@@ -1,22 +1,37 @@
-// RUN: %clang -fintegrated-cc1 -### %s 2>&1 | FileCheck %s --check-prefix=YES
-// RUN: %clang -fno-integrated-cc1 -### %s 2>&1 | FileCheck %s --check-prefix=NO
+// RUN: %clang -fintegrated-cc1 -c -### %s 2>&1 | FileCheck %s --check-prefix=YES
+// RUN: %clang -fno-integrated-cc1 -c -### %s 2>&1 | FileCheck %s --check-prefix=NO
 
-// RUN: %clang -fintegrated-cc1 -fno-integrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fintegrated-cc1 -fno-integrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=NO
-// RUN: %clang -fno-integrated-cc1 -fintegrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fno-integrated-cc1 -fintegrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES
 
-// RUN: %clang_cl -fintegrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fintegrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES
-// RUN: %clang_cl -fno-integrated-cc1 -### -- %s 2>&1 \
+// RUN: %clang_cl -fno-integrated-cc1 -c -### -- %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=NO
 
 // RUN: env CCC_OVERRIDE_OPTIONS=+-fintegrated-cc1 \
-// RUN: %clang -fintegrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fintegrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=YES
 // RUN: env CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1 \
-// RUN: %clang -fintegrated-cc1 -### %s 2>&1 \
+// RUN: %clang -fintegrated-cc1 -c -### %s 2>&1 \
 // RUN: | FileCheck %s --check-prefix=NO
 
 // YES: (in-process)
 // NO-NOT: (in-process)
+
+// The following tests ensure that only one integrated-cc1 is executed.
+
+// Only one TU, one job, thus integrated-cc1 is enabled.
+// RUN: %clang -fintegrated-cc1 -c %s -### 2>&1 | FileCheck %s --check-prefix=YES
+
+// Only one TU, but we're linking, two jobs, thus integrated-cc1 is disabled.
+// RUN: %clang -fintegrated-cc1 %s -### 2>&1 | FileCheck %s --check-prefix=NO
+
+// RUN: echo 'int main() { return f() + g(); }' > %t1.cpp
+// RUN: echo 'int f() { return 1; }' > %t2.cpp
+// RUN: echo 'int g() { return 2; }' > %t3.cpp
+
+// Three jobs, thus integrated-cc1 is disabled.
+// RUN: %clang -fintegrated-cc1 -c %t1.cpp %t2.cpp %t3.cpp -### 2>&1 | FileCheck %s --check-prefix=NO
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -6147,7 +6147,7 @@
   if (Output.getType() == types::TY_Object &&
   Args.hasFlag(options::OPT__SLASH_showFilenames,
options::OPT__SLASH_showFilenames_, false)) {
-C.getJobs().getJobs().back()->setPrintInputFilenames(true);
+C.getJobs().getJobs().back()->PrintInputFilenames = true;
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
Index: clang/lib/Driver/Job.cpp
===
--- clang/lib/Driver/Job.cpp
+++ clang/lib/Driver/Job.cpp
@@ -40,7 +40,7 @@
  const llvm::opt::ArgStringList ,
  ArrayRef Inputs)
 : Source(Source), Creator(Creator),