[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher created this revision.
delcypher added reviewers: kubamracek, yln, aralisza, beanz, samsonov.
Herald added subscribers: mgorny, dberris.
delcypher requested review of this revision.
Herald added a project: clang.

When building with `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON` (e.g. Swift does
this) we do an "external" build of compiler-rt where we build
compiler-rt with the just built clang.

Unfortunately building in this mode had a bug where compiler-rt would
not get rebuilt if compiler-rt sources changed. This is problematic
for incremental builds because it meant that the compiler-rt binaries
were stale.

The fix is to use the `BUILD_ALWAYS` ExternalProject_Add option which
means the build command for compiler-rt is always run.

If all of the following are true:

- compiler-rt has already been built.
- there are no compiler-rt source changes.
- ninja is being used as the generator for the compiler-rt build.

then the overhead for always running the build command for incremental
builds is negligible.

rdar://75150660


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98291

Files:
  clang/runtime/CMakeLists.txt


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher updated this revision to Diff 329469.
delcypher added a comment.

- update description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

Files:
  clang/runtime/CMakeLists.txt


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Kuba (Brecka) Mracek via Phabricator via cfe-commits]

kubamracek added a comment.

When using Ninja, does this mean running a null build is no longer a null 
build, but it at least always invokes this sub-build? Not saying that's bad, 
just want to understand the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Chris Bieneman via Phabricator via cfe-commits]

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

So... this patch is fine. (marked as approved). That said, we should really be 
working to remove the `LLVM_BUILD_EXTERNAL_COMPILER_RT` option entirely in 
favor of just using `LLVM_ENABLE_RUNTIMES=compiler-rt`. That build process does 
support invoking the sub-project build on build invocation to make sure builds 
get re-run, and it supports better understanding of the dependencies between 
runtimes.

Have you considered using that instead of `LLVM_BUILD_EXTERNAL_COMPILER_RT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D98291#2615590 , @kubamracek wrote:

> When using Ninja, does this mean running a null build is no longer a null 
> build, but it at least always invokes this sub-build? Not saying that's bad, 
> just want to understand the change. What does the output of just "ninja" say 
> in that case?

Yes the sub-build will always be invoked. That being said in the sub-build if 
nothing has changed it will be extremely fast because ninja is fast.

The output looks something like this

  $ ninja
  [0/3] Performing build step for 'compiler-rt'
  ninja: no work to do.
  [1/3] No install step for 'compiler-rt'
  [3/3] Completed 'compiler-rt'

This being said I'd expect very few people to actually run into this because 
this build configuration (`LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`) is not common 
because it is not the default. The default if you enable compiler-rt is for it 
to be built as part of the main build using the host compiler, which is all 
kinds of wrong but is very convenient.

Swift does use this build configuration but I doubt anyone would complain about 
the extra console noise given how noisy build-script is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D98291#2615593 , @beanz wrote:

> So... this patch is fine. (marked as approved). That said, we should really 
> be working to remove the `LLVM_BUILD_EXTERNAL_COMPILER_RT` option entirely in 
> favor of just using `LLVM_ENABLE_RUNTIMES=compiler-rt`. That build process 
> does support invoking the sub-project build on build invocation to make sure 
> builds get re-run, and it supports better understanding of the dependencies 
> between runtimes.
>
> Have you considered using that instead of `LLVM_BUILD_EXTERNAL_COMPILER_RT`?

@beanz It's something I've thought about but haven't had the time to do. The 
fact that we have 3 different ways to build compiler-rt in LLVM makes me sad. 
There should only be 1. I don't have time to do it now but I've filed a radar 
to do this work at some point (rdar://75248447).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D98291#2615663 , @delcypher wrote:

> The fact that we have 3 different ways to build compiler-rt in LLVM makes me 
> sad. There should only be 1. I don't have time to do it now but I've filed a 
> radar to do this work at some point (rdar://75248447).

+1,000

The runtimes directory is really the only "correct" way to build the runtime 
libraries. Someday I hope we can get everything building through it.

In the meantime since reality is a thing... this patch is a good bug fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Kuba (Brecka) Mracek via Phabricator via cfe-commits]

kubamracek accepted this revision.
kubamracek added a comment.

Looks good!

By the way, Apple Clang releases also build compiler-rt this way, so it would 
be worth checking with @arphaman that he's okay with the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa159f91c8d06: [compiler-rt] Fix stale incremental builds 
when using… (authored by delcypher).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

Files:
  clang/runtime/CMakeLists.txt


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)


Index: clang/runtime/CMakeLists.txt
===
--- clang/runtime/CMakeLists.txt
+++ clang/runtime/CMakeLists.txt
@@ -95,6 +95,8 @@
 USES_TERMINAL_CONFIGURE 1
 USES_TERMINAL_BUILD 1
 USES_TERMINAL_INSTALL 1
+# Always run the build command so that incremental builds are correct.
+BUILD_ALWAYS 1
 )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D98291#2616865 , @kubamracek wrote:

> Looks good!
>
> By the way, Apple Clang releases also build compiler-rt this way, so it would 
> be worth checking with @arphaman that he's okay with the change.

@kubamracek @arphaman Ah sorry I landed the change before I saw this message. 
It looks like the `LLVM_BUILD_EXTERNAL_COMPILER_RT` option is used as part of 
the 2-stage boot strap for AppleClang as it's set in 
`clang/cmake/caches/Apple-stage2.cmake`. I'll follow up offline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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

[PATCH] D98291: [compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

This should not impact the Apple Clang releases since those are always clean 
builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98291

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