[LLVM] Remove explicit dependency on builtins build

[jhuber6]

Summary:
The LLVM runtimes build internally uses ExternalProject_Add. This then
uses BUILD_ALWAYS to force the targets to be up-to-date when needed.
One problem with this is that we have the builtins step and the runtimes
step, which depends on the former. This means that when we build, we
always touch the builtins, which then makes the runtimes stale.

This patch tries to weaken that relationship by just getting rid of the
explicit dependency. Because these targets are already BUILD_ALWAYS
the compiler-rt target will always be built before the runtimes. This
means it will always be available in-order when doing a normal build.

What this patch changes is that now if someone makes a significant
change to the compiler-rt runtime implementation it now may not
trigger a rebuild of every single source file. I'm not sure if this
behavior was here previously in the first place either, since it was
just using a stale dependency and never did any extra work. I'm pretty
sure most of the uses of the library are implicit through clang so it
probably wouldn't track that dependency right anyway.

I cold be wrong, but I'm hoping this resolves an issue I've had for a
long time which makes it prohibitively difficult to use the runtimes
interface with compiler-rt enabled as a runtime.

Fixes: #98897

[jhuber6]

Ping, I'm fairly certain this is equivalent to what we already do since the compiler-rt dependency is implicit. Because we BUILD_ALWAYS it should always be done first and never be out-of-date. At least that's what I've observed. I'm extremely eager to resolve this as this issue currently causes a trivial whitespace fix to take 60 seconds to reconfigure all of CMake.

[Meinersbur]

I don't know the compiler-rt/builtins system well enough to know whether theses dependencies can just be removed. Would't it be more elegant to avoid BUILD_ALWAYS in the first place?

[jhuber6]

I don't know the compiler-rt/builtins system well enough to know whether theses dependencies can jsut be removed. Would't it be more elegant to avoid BUILD_ALWAYS in the first place?

I tried that, but as far as I can tell the BUILD_ALWAYS is required to keep the runtimes correctly up-to-date. CMake doesn't know anything about the internal targets. We could theoretically export stamps manually, but since there's tons of targets that vary by what the user requested it'd be quite difficult to rig together.

In practice if you remove BUILD_ALWAYS and modify the runtimes source, you will need to manually retrigger a build from the runtimes directory, the root one will just say 'no work to do'.

[petrhosek]

I tried change locally and looked at the generated Ninja file but I don't see any dependency edge between builtins-* and runtimes-* targets; while runtimes/all depends on both builtins-* and runtimes-* targets, there's no guarantee that former will be built before the latter, so I'm not convinced this change is correct and it may be working purely by luck.

I think the correct solution would be to avoid relying on DEPENDS in ExternalProject_Add which adds a dependency to all steps and instead use ExternalProject_Add_StepDependencies to add fine-grained dependencies between targets that need them, but that's going to need some refactoring in LLVMExternalProjectUtils.cmake.

[jhuber6]

I tried change locally and looked at the generated Ninja file but I don't see any dependency edge between builtins-* and runtimes-* targets; while runtimes/all depends on both builtins-* and runtimes-* targets, there's no guarantee that former will be built before the latter, so I'm not convinced this change is correct and it may be working purely by luck.

Yeah, I'm going to assume that things in the all target are built in some 'undetermined' order, which is probably just the order they were added. But, it doesn't seem to mandate this anywhere so we can't technically depend on it.

I think the correct solution would be to avoid relying on DEPENDS in ExternalProject_Add which adds a dependency to all steps and instead use ExternalProject_Add_StepDependencies to add fine-grained dependencies between targets that need them, but that's going to need some refactoring in LLVMExternalProjectUtils.cmake.

The problem is that the dependency edge is always considered stale. I suppose this method would let us correctly inform the step of dependencies without the build always functionality? I could try looking at it, because I'm eager to fix this since it has probably cost me literal days of unnecessary rebuild time as a trivial change that should take 2 seconds instead takes 90 seconds.

[jhuber6]

I'm not sure separating the steps will help unless I'm missing something. The builtins and runtimes builds are part of the same build target as far as I know. The fundamental problem occurs because we cannot do proper dependency management between this barrier. Normally you'd have the runtimes depend on the builtins, but because of the BUILD_ALWAYS usage this is always out of date. You can get rid of BUILD_ALWAYS but then there's no way to inform the top level CMake that the dependencies are stale.