[CMake] Serialize `build_native_tool`

[chapuni]

Summary

To prevent race in NATIVE, let each action depend on preceding action.

For example,

• llvm-tblgen-host depends on the target's llvm-tblgen.
• clang-tblgen-host depends on the target's clang-tblgen and llvm-tblgen-host.

This is rework for D54153.
build_native_tool has been introduced since D60024.

[chapuni]

I have checked with just only Ninja generator.

[chapuni]

• Add a comment line

[mstorsjo]

I think this looks reasonable, and the logic seems mostly better than the one before. As far as I can see with the current code in git, there's nothing stopping from building e.g. lldb-tablegen-host and clang-tablegen-host at the same time, is that right?

This code works best if the tablegens are processed in a roughly topological order, e.g. in a build with clang and lldb, I guess it's better if clang is listed before lldb in LLVM_ENABLE_PROJECTS, because otherwise clang-tablegen-host would build lldb-tablegen-host before?

[chapuni]

In D149072#4296385, @mstorsjo wrote:

I think this looks reasonable, and the logic seems mostly better than the one before. As far as I can see with the current code in git, there's nothing stopping from building e.g. lldb-tablegen-host and clang-tablegen-host at the same time, is that right?

I haven't experienced lldb, though, I guess lldb-tablegen-host and clang-tablegen-host would be scheduled in parallel.

This code works best if the tablegens are processed in a roughly topological order, e.g. in a build with clang and lldb, I guess it's better if clang is listed before lldb in LLVM_ENABLE_PROJECTS, because otherwise clang-tablegen-host would build lldb-tablegen-host before?

I think such optimization would not gain the big win. Nested CMake invocation would be evil and far from optimal I suppose.
I just wanted to introduce consistency.

That said, I will not stop someone's proposing better scheduling.

[mstorsjo]

In D149072#4299252, @chapuni wrote:

This code works best if the tablegens are processed in a roughly topological order, e.g. in a build with clang and lldb, I guess it's better if clang is listed before lldb in LLVM_ENABLE_PROJECTS, because otherwise clang-tablegen-host would build lldb-tablegen-host before?

I think such optimization would not gain the big win.

I can try to check if this happens or not.

Nested CMake invocation would be evil and far from optimal I suppose.

I don't quite understand what you refer to here?

I just wanted to introduce consistency.

That said, I will not stop someone's proposing better scheduling.

I don't think it's necessarily worth spending effort on at this point, I just wanted to note the pros and cons. Doing things suboptimally (in a case where it's trivial for the user to fix it) is totally fine, compared to doing things in an unspecified possibly broken way.

[beanz]

This feels like it could have unintended and unfortunate consequences. Does this not make mlir-tablegen depend on clang-tablegen? If it does that could increase iteration loops where native tools are involved by building unrelated things that don't strictly need to be built or have a dependency.

I'm curious where you're seeing race conditions with the native builds. Makefile and Ninja generators shouldn't race (makefile because of the job server model and Ninja because of the use of the console pool).

[chapuni]

In D149072#4300868, @beanz wrote:

This feels like it could have unintended and unfortunate consequences.

I know this is far from optimal rather than suboptimal. I don't intend any optimizations.

Does this not make mlir-tablegen depend on clang-tablegen? If it does that could increase iteration loops where native tools are involved by building unrelated things that don't strictly need to be built or have a dependency.

I know this will introduce longer dependency chain. That said, subsequent NATIVE's actions would not rebuild preceding artifacts (unless the builder would be silly).

I'm curious where you're seeing race conditions with the native builds. Makefile and Ninja generators shouldn't race (makefile because of the job server model and Ninja because of the use of the console pool).

I have just followed D54153 and I haven't met any matters in my use cases. I haven't thought this logic could be optimized (in this).

At the moment I would like to land this unconditionally. I think we could exclude this conditionally later.

[beanz]

This will regress build times for Makefile and Ninja builds, particularly clean builds (which is what GitHub Actions, Azure Pipelines, and Google Cloud Pipelines all do).

Please restrict the change to only impact Visual Studio which I think is the only generator that can be impacted by the race condition. Xcode may be sub-optimal with the current behavior but probably isn't impacted by the race because the filesystem locking behavior is likely NTFS-specific.

[chapuni]

• Restrict the change only for "Visual Studio"

[beanz]

LGTM. Thank you!

llvm/cmake/modules/CrossCompile.cmake

function(build_native_tool target output_path_var)
 
   if(CMAKE_CONFIGURATION_TYPES)
     set(output_path "${${PROJECT_NAME}_NATIVE_BUILD}/Release/bin/${target}")
   else()
     set(output_path "${${PROJECT_NAME}_NATIVE_BUILD}/bin/${target}")
   endif()
   set(output_path ${output_path}${LLVM_HOST_EXECUTABLE_SUFFIX})
 
+  # Make chain of preceding actions
+  if(CMAKE_GENERATOR MATCHES "Visual Studio")
+    get_property(host_targets GLOBAL PROPERTY ${PROJECT_NAME}_HOST_TARGETS)
+    set_property(GLOBAL APPEND PROPERTY ${PROJECT_NAME}_HOST_TARGETS ${output_path})
+  endif()
+
   llvm_ExternalProject_BuildCmd(build_cmd ${target} ${${PROJECT_NAME}_NATIVE_BUILD}
                                 CONFIGURATION Release)
   add_custom_command(OUTPUT "${output_path}"
                      COMMAND ${build_cmd}
-                     DEPENDS CONFIGURE_${PROJECT_NAME}_NATIVE ${ARG_DEPENDS}
+                     DEPENDS CONFIGURE_${PROJECT_NAME}_NATIVE ${ARG_DEPENDS} ${host_targets}
                      WORKING_DIRECTORY "${${PROJECT_NAME}_NATIVE_BUILD}"
                      COMMENT "Building native ${target}..."
                      USES_TERMINAL)
   set(${output_path_var} "${output_path}" PARENT_SCOPE)
 endfunction()

llvm/cmake/modules/TableGen.cmake

if( ${${project}_TABLEGEN} STREQUAL "${target}" )
       # otherwise the native one won't get rebuilt when the tablgen sources
       # change, and we end up with incorrect builds.
       build_native_tool(${target} ${project}_TABLEGEN_EXE DEPENDS ${target})
       set(${project}_TABLEGEN_EXE ${${project}_TABLEGEN_EXE} PARENT_SCOPE)
 
       add_custom_target(${project}-tablegen-host DEPENDS ${${project}_TABLEGEN_EXE})
       set(${project}_TABLEGEN_TARGET ${project}-tablegen-host PARENT_SCOPE)
 
-      # Create an artificial dependency between tablegen projects, because they
-      # compile the same dependencies, thus using the same build folders.
-      # FIXME: A proper fix requires sequentially chaining tablegens.
-      if (NOT ${project} STREQUAL LLVM AND TARGET ${project}-tablegen-host AND
-          TARGET LLVM-tablegen-host)
-        add_dependencies(${project}-tablegen-host LLVM-tablegen-host)
-      endif()
-
       # If we're using the host tablegen, and utils were not requested, we have no
       # need to build this tablegen.
       if ( NOT LLVM_BUILD_UTILS )
         set_target_properties(${target} PROPERTIES EXCLUDE_FROM_ALL ON)
       endif()
     endif()
   endif()