[CMake][TableGen] Make TableGen CMake functions compatible with CMP0116 #72333

akirchhoff-modular · 2023-11-15T01:04:19Z

By default, add_custom_command rules execute within their defining directory's corresponding build directory. When DEPFILE is used with Ninja, this causes a discrepancy: Ninja expects the path in the depfile to be a relative path from the root of the build directory, but since the working directory is changed by default during the execution of the rule, the path won't match up, and Ninja will conservatively ignore the depfile.

Historically, this is the only way it worked, and CMakeLists.txt files would have to be written to work around this, manually converting paths to build-root-directory-relative and changing the working directory of the command to match.

Starting with CMake 3.20, CMake introduced a native workaround, controlled by policy CMP0116. When CMP0116 is set to NEW, CMake will automatically translate depfiles such that CMakeLists.txt no longer need to be written taking this into account. This behavior is incompatible with the manual workaround. LLVM sets CMP0116 to OLD in cmake/Modules/CMakePolicy.cmake, so when building LLVM, this is not a problem -- we turn off CMake's native workaround and work around it ourselves.

Since the TableGen CMake modules are also installed along with LLVM, downstream projects that use LLVM and its TableGen CMake rules are also forced to adopt CMP0116 to be set to OLD. This causes conflicts when other dependencies of such downstream projects require CMP0116 to be set to NEW. CMake policy scopes can help with this, but it is tedious to handle, as a policy scope would need to be introduced whenever the TableGen rule is used, and it requires the caller to understand an implementation detail of the TableGen rule.

Instead, this PR changes the TableGen CMake rules such that they will inspect the current state of CMP0116, and only conditionally apply the manual workaround. This way, the TableGen rules will work regardless of the setting of CMP0116.

In the future, potentially the value of CMP0116 set in CMakePolicy.cmake could change to bring LLVM up-to-date with CMake default behavior. However, I am leaving out such a default change to reduce any possibility of breakage for other downstream projects at this time.

By default, `add_custom_command` rules execute within their defining directory's corresponding build directory. When `DEPFILE` is used with Ninja, this causes a discrepancy: Ninja expects the path in the depfile to be a relative path from the root of the build directory, but since the working directory is changed by default during the execution of the rule, the path won't match up, and Ninja will conservatively ignore the depfile. Historically, this is the only way it worked, and CMakeLists.txt files would have to be written to work around this, manually converting paths to build-root-directory-relative and changing the working directory of the command to match. Starting with CMake 3.20, CMake introduced a native workaround, controlled by policy CMP0116. When CMP0116 is set to NEW, CMake will automatically translate depfiles such that CMakeLists.txt no longer need to be written taking this into account. This behavior is incompatible with the manual workaround. LLVM sets CMP0116 to OLD in `cmake/Modules/CMakePolicy.cmake`, so when building LLVM, this is not a problem -- we turn off CMake's native workaround and work around it ourselves. Since the TableGen CMake modules are also installed along with LLVM, downstream projects that use LLVM and its TableGen CMake rules are also forced to adopt CMP0116 to be set to OLD. This causes conflicts when other dependencies of such downstream projects require CMP0116 to be set to NEW. CMake policy scopes can help with this, but it is tedious to handle, as a policy scope would need to be introduced whenever the TableGen rule is used, and it requires the caller to understand an implementation detail of the TableGen rule. Instead, this PR changes the TableGen CMake rules such that they will inspect the current state of CMP0116, and only conditionally apply the manual workaround. This way, the TableGen rules will work regardless of the setting of CMP0116. In the future, potentially the value of CMP0116 set in `CMakePolicy.cmake` could change to bring LLVM up-to-date with CMake default behavior. However, I am leaving out such a default change to reduce any possibility of breakage for other downstream projects at this time.

llvmbot · 2023-11-15T01:04:49Z

@llvm/pr-subscribers-mlir

Author: None (akirchhoff-modular)

Changes

By default, add_custom_command rules execute within their defining directory's corresponding build directory. When DEPFILE is used with Ninja, this causes a discrepancy: Ninja expects the path in the depfile to be a relative path from the root of the build directory, but since the working directory is changed by default during the execution of the rule, the path won't match up, and Ninja will conservatively ignore the depfile.

Historically, this is the only way it worked, and CMakeLists.txt files would have to be written to work around this, manually converting paths to build-root-directory-relative and changing the working directory of the command to match.

Starting with CMake 3.20, CMake introduced a native workaround, controlled by policy CMP0116. When CMP0116 is set to NEW, CMake will automatically translate depfiles such that CMakeLists.txt no longer need to be written taking this into account. This behavior is incompatible with the manual workaround. LLVM sets CMP0116 to OLD in cmake/Modules/CMakePolicy.cmake, so when building LLVM, this is not a problem -- we turn off CMake's native workaround and work around it ourselves.

Since the TableGen CMake modules are also installed along with LLVM, downstream projects that use LLVM and its TableGen CMake rules are also forced to adopt CMP0116 to be set to OLD. This causes conflicts when other dependencies of such downstream projects require CMP0116 to be set to NEW. CMake policy scopes can help with this, but it is tedious to handle, as a policy scope would need to be introduced whenever the TableGen rule is used, and it requires the caller to understand an implementation detail of the TableGen rule.

Instead, this PR changes the TableGen CMake rules such that they will inspect the current state of CMP0116, and only conditionally apply the manual workaround. This way, the TableGen rules will work regardless of the setting of CMP0116.

In the future, potentially the value of CMP0116 set in CMakePolicy.cmake could change to bring LLVM up-to-date with CMake default behavior. However, I am leaving out such a default change to reduce any possibility of breakage for other downstream projects at this time.

Full diff: https://github.com/llvm/llvm-project/pull/72333.diff

2 Files Affected:

(modified) llvm/cmake/modules/TableGen.cmake (+27-12)
(modified) mlir/cmake/modules/AddMLIR.cmake (+27-12)

diff --git a/llvm/cmake/modules/TableGen.cmake b/llvm/cmake/modules/TableGen.cmake
index 7fd6628ef55d33a..0f6aa0c5d826ab3 100644
--- a/llvm/cmake/modules/TableGen.cmake
+++ b/llvm/cmake/modules/TableGen.cmake
@@ -19,19 +19,34 @@ function(tablegen project ofn)
 
   # Use depfile instead of globbing arbitrary *.td(s) for Ninja.
   if(CMAKE_GENERATOR MATCHES "Ninja")
-    # Make output path relative to build.ninja, assuming located on
-    # ${CMAKE_BINARY_DIR}.
     # CMake emits build targets as relative paths but Ninja doesn't identify
-    # absolute path (in *.d) as relative path (in build.ninja)
-    # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
-    file(RELATIVE_PATH ofn_rel
-      ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
-    set(additional_cmdline
-      -o ${ofn_rel}
-      -d ${ofn_rel}.d
-      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
-      DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
-      )
+    # absolute path (in *.d) as relative path (in build.ninja).  If CMP0116 is
+    # NEW, CMake handles this discrepancy for us -- otherwise, we have to work
+    # around it ourselves.
+    if(POLICY CMP0116)
+      cmake_policy(GET CMP0116 cmp0116_state)
+    else()
+      set(cmp0116_state OLD)
+    endif()
+    if(cmp0116_state STREQUAL NEW)
+      set(additional_cmdline
+        -o ${ofn}
+        -d ${ofn}.d
+        DEPFILE ${ofn}.d
+        )
+    else()
+      # Make output path relative to build.ninja, assuming located on
+      # ${CMAKE_BINARY_DIR}.
+      # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
+      file(RELATIVE_PATH ofn_rel
+        ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
+      set(additional_cmdline
+        -o ${ofn_rel}
+        -d ${ofn_rel}.d
+        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+        DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
+        )
+    endif()
     set(local_tds)
     set(global_tds)
   else()
diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index 544abe43688820e..714054f17515ca7 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -48,19 +48,34 @@ function(_pdll_tablegen project ofn)
 
   # Use depfile instead of globbing arbitrary *.td(s) for Ninja.
   if(CMAKE_GENERATOR MATCHES "Ninja")
-    # Make output path relative to build.ninja, assuming located on
-    # ${CMAKE_BINARY_DIR}.
     # CMake emits build targets as relative paths but Ninja doesn't identify
-    # absolute path (in *.d) as relative path (in build.ninja)
-    # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
-    file(RELATIVE_PATH ofn_rel
-      ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
-    set(additional_cmdline
-      -o ${ofn_rel}
-      -d ${ofn_rel}.d
-      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
-      DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
-      )
+    # absolute path (in *.d) as relative path (in build.ninja).  If CMP0116 is
+    # NEW, CMake handles this discrepancy for us -- otherwise, we have to work
+    # around it ourselves.
+    if(POLICY CMP0116)
+      cmake_policy(GET CMP0116 cmp0116_state)
+    else()
+      set(cmp0116_state OLD)
+    endif()
+    if(cmp0116_state STREQUAL NEW)
+      set(additional_cmdline
+        -o ${ofn}
+        -d ${ofn}.d
+        DEPFILE ${ofn}.d
+        )
+    else()
+      # Make output path relative to build.ninja, assuming located on
+      # ${CMAKE_BINARY_DIR}.
+      # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
+      file(RELATIVE_PATH ofn_rel
+        ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
+      set(additional_cmdline
+        -o ${ofn_rel}
+        -d ${ofn_rel}.d
+        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+        DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
+        )
+    endif()
     set(local_tds)
     set(global_tds)
   else()

chapuni

I suggest, in this opportunity;

Drop cmake_policy(SET CMP0116 OLD) to follow the new policy
Drop the old logic
Anyways check if CMP0116 is NEW, otherwise use the fallback logic.

I suppose users of LLVM (as a module) may assume using CMake>=3.20.

chapuni · 2023-11-16T14:26:32Z

llvm/cmake/modules/TableGen.cmake

+    # absolute path (in *.d) as relative path (in build.ninja).  If CMP0116 is
+    # NEW, CMake handles this discrepancy for us -- otherwise, we have to work
+    # around it ourselves.
+    if(POLICY CMP0116)


Could we omit it since we use CMake>=3.20?

chapuni · 2023-11-16T14:30:00Z

llvm/cmake/modules/TableGen.cmake

+      # Make output path relative to build.ninja, assuming located on
+      # ${CMAKE_BINARY_DIR}.
+      # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
+      file(RELATIVE_PATH ofn_rel
+        ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
+      set(additional_cmdline
+        -o ${ofn_rel}
+        -d ${ofn_rel}.d
+        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+        DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
+        )


I think we may rely on the new policy and drop old logic.
Instead, we could delegate to fallback logic file(GLOB).

chapuni · 2023-11-16T14:32:17Z

llvm/cmake/modules/TableGen.cmake

@@ -19,19 +19,34 @@ function(tablegen project ofn)

  # Use depfile instead of globbing arbitrary *.td(s) for Ninja.
  if(CMAKE_GENERATOR MATCHES "Ninja")


if(CMAKE_GENERATOR MATCHES "Ninja" AND cmp0116_state STREQUAL "NEW")

I think it would be fair to fall back to file(GLOB) if CMP0116 is OLD.

CMake emits build targets as relative paths (from build.ninja) but Ninja doesn't identify absolute path (in *.d) as relative path (in build.ninja). So, let file names, in the command line, relative from ${CMAKE_BINARY_DIR}, where build.ninja is. Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory. Differential Revision: https://reviews.llvm.org/D33707 llvm-svn: 305961

boomanaiden154 · 2024-04-30T03:51:04Z

@akirchhoff-modular are you still interested in driving this? I'd like to be able to not force the policies to OLD anymore (see #90385/the associate issue), and this is one of the issues that came up. If not, do you mind if I pick up the patch?

akirchhoff-modular · 2024-04-30T18:04:45Z

@akirchhoff-modular are you still interested in driving this? I'd like to be able to not force the policies to OLD anymore (see #90385/the associate issue), and this is one of the issues that came up. If not, do you mind if I pick up the patch?

@boomanaiden154 Please go ahead and pick this up if you're interested. I still think this should happen but priorities have shifted at work such that I haven't had time to get back to this PR.

akirchhoff-modular added cmake Build system in general and CMake in particular tablegen labels Nov 15, 2023

akirchhoff-modular requested a review from chapuni November 15, 2023 01:04

llvmbot added the mlir label Nov 15, 2023

JoeLoser approved these changes Nov 15, 2023

View reviewed changes

chapuni requested review from kastiglione and mgorny November 16, 2023 14:25

chapuni requested changes Nov 16, 2023

View reviewed changes

zero9178 mentioned this pull request Apr 28, 2024

[CMake] Do not set CMP0116 explicitly to old #90385

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMake][TableGen] Make TableGen CMake functions compatible with CMP0116 #72333

[CMake][TableGen] Make TableGen CMake functions compatible with CMP0116 #72333

akirchhoff-modular commented Nov 15, 2023

llvmbot commented Nov 15, 2023

chapuni left a comment

chapuni Nov 16, 2023

chapuni Nov 16, 2023

chapuni Nov 16, 2023

boomanaiden154 commented Apr 30, 2024

akirchhoff-modular commented Apr 30, 2024

		@@ -19,19 +19,34 @@ function(tablegen project ofn)

		# Use depfile instead of globbing arbitrary *.td(s) for Ninja.
		if(CMAKE_GENERATOR MATCHES "Ninja")

[CMake][TableGen] Make TableGen CMake functions compatible with CMP0116 #72333

Are you sure you want to change the base?

[CMake][TableGen] Make TableGen CMake functions compatible with CMP0116 #72333

Conversation

akirchhoff-modular commented Nov 15, 2023

llvmbot commented Nov 15, 2023

chapuni left a comment

Choose a reason for hiding this comment

chapuni Nov 16, 2023

Choose a reason for hiding this comment

chapuni Nov 16, 2023

Choose a reason for hiding this comment

chapuni Nov 16, 2023

Choose a reason for hiding this comment

boomanaiden154 commented Apr 30, 2024

akirchhoff-modular commented Apr 30, 2024