[Offload] Correctly regenerate API files if modified #141679

jhuber6 · 2025-05-27T22:05:43Z

Summary:
Currently these are done POST_BUILD, but they should be dependencies on
generating the library since it's a direct dependency.

llvmbot · 2025-05-27T22:06:17Z

@llvm/pr-subscribers-offload

Author: Joseph Huber (jhuber6)

Changes

Summary:
Currently these are done POST_BUILD, but they should be dependencies on
generating the library since it's a direct dependency.

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

2 Files Affected:

(modified) offload/liboffload/API/CMakeLists.txt (+17-10)
(modified) offload/liboffload/CMakeLists.txt (+10-5)

diff --git a/offload/liboffload/API/CMakeLists.txt b/offload/liboffload/API/CMakeLists.txt
index 5f8d1435d141f..b3b499568fe84 100644
--- a/offload/liboffload/API/CMakeLists.txt
+++ b/offload/liboffload/API/CMakeLists.txt
@@ -2,8 +2,8 @@
 # include directory. These files are checked in with the rest of the source,
 # therefore it is only needed when making changes to the API.
 
-find_program(CLANG_FORMAT clang-format PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
-if (CLANG_FORMAT)
+find_program(clang_format clang-format PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
+if (clang_format)
     set(LLVM_TARGET_DEFINITIONS ${CMAKE_CURRENT_SOURCE_DIR}/OffloadAPI.td)
 
     tablegen(OFFLOAD OffloadAPI.h -gen-api)
@@ -13,15 +13,22 @@ if (CLANG_FORMAT)
     tablegen(OFFLOAD OffloadPrint.hpp -gen-print-header)
     tablegen(OFFLOAD OffloadErrcodes.inc -gen-errcodes)
 
-    set(FILES_TO_COPY "OffloadAPI.h;OffloadEntryPoints.inc;OffloadFuncs.inc;OffloadImplFuncDecls.inc;OffloadPrint.hpp")
-    set(GEN_DIR ${CMAKE_CURRENT_SOURCE_DIR}/../include/generated)
+    set(files_to_copy "OffloadAPI.h;OffloadEntryPoints.inc;OffloadFuncs.inc;OffloadImplFuncDecls.inc;OffloadPrint.hpp")
+    set(generated_dir ${CMAKE_CURRENT_SOURCE_DIR}/../include/generated)
     add_public_tablegen_target(OffloadGenerate)
-    add_custom_command(TARGET OffloadGenerate POST_BUILD COMMAND ${CLANG_FORMAT}
-        -i ${TABLEGEN_OUTPUT})
-    add_custom_command(TARGET OffloadGenerate POST_BUILD COMMAND ${CMAKE_COMMAND}
-        -E copy_if_different ${FILES_TO_COPY} ${GEN_DIR})
-    add_custom_command(TARGET OffloadGenerate POST_BUILD COMMAND ${CMAKE_COMMAND}
-        -E copy_if_different OffloadErrcodes.inc "${LIBOMPTARGET_INCLUDE_DIR}/Shared/OffloadErrcodes.inc")
+
+    add_custom_target(OffloadAPI DEPENDS OffloadGenerate)
+    foreach(file IN LISTS files_to_copy)
+      add_custom_command(
+          OUTPUT ${generated_dir}/${file}
+          COMMAND ${clang_format} -i ${file}
+          COMMAND ${CMAKE_COMMAND} -E make_directory ${generated_dir}
+          COMMAND ${CMAKE_COMMAND} -E copy_if_different ${file} ${generated_dir}
+          DEPENDS ${file}
+      )
+      add_custom_target(OffloadAPI.${file} DEPENDS ${generated_dir}/${file})
+      add_dependencies(OffloadAPI OffloadAPI.${file})
+    endforeach()
 else()
     message(WARNING "clang-format was not found, so the OffloadGenerate target\
         will not be available. Offload will still build, but you will not be\
diff --git a/offload/liboffload/CMakeLists.txt b/offload/liboffload/CMakeLists.txt
index 1b098bc01e218..6dcc5b430a0b6 100644
--- a/offload/liboffload/CMakeLists.txt
+++ b/offload/liboffload/CMakeLists.txt
@@ -19,11 +19,11 @@ if(LLVM_HAVE_LINK_VERSION_SCRIPT)
 endif()
 
 target_include_directories(LLVMOffload PUBLIC
-                            ${CMAKE_CURRENT_BINARY_DIR}/../include
-                            ${CMAKE_CURRENT_SOURCE_DIR}/include
-                            ${CMAKE_CURRENT_SOURCE_DIR}/include/generated
-                            ${CMAKE_CURRENT_SOURCE_DIR}/../include
-                            ${CMAKE_CURRENT_SOURCE_DIR}/../plugins-nextgen/common/include)
+                           ${CMAKE_CURRENT_BINARY_DIR}/../include
+                           ${CMAKE_CURRENT_SOURCE_DIR}/include
+                           ${CMAKE_CURRENT_SOURCE_DIR}/include/generated
+                           ${CMAKE_CURRENT_SOURCE_DIR}/../include
+                           ${CMAKE_CURRENT_SOURCE_DIR}/../plugins-nextgen/common/include)
 
 target_compile_options(LLVMOffload PRIVATE ${offload_compile_flags})
 target_link_options(LLVMOffload PRIVATE ${offload_link_flags})
@@ -33,6 +33,11 @@ target_compile_definitions(LLVMOffload PRIVATE
   DEBUG_PREFIX="Liboffload"
 )
 
+# Make sure these are up-to-date if modified.
+if(TARGET OffloadAPI)
+  add_dependencies(LLVMOffload OffloadAPI)
+endif()
+
 set_target_properties(LLVMOffload PROPERTIES
                       POSITION_INDEPENDENT_CODE ON
                       INSTALL_RPATH "$ORIGIN"

jhuber6 · 2025-05-27T22:09:09Z

@callumfare

callumfare

Thanks, one comment but otherwise it looks good.

At this point I'm not against removing the checked-in generated files to conform with the way Tablegen is used elsewhere, if that's something we'd prefer. The clang-format step could be optional in that case.

callumfare · 2025-05-28T08:56:06Z

offload/liboffload/API/CMakeLists.txt

-    add_custom_command(TARGET OffloadGenerate POST_BUILD COMMAND ${CMAKE_COMMAND}
-        -E copy_if_different ${FILES_TO_COPY} ${GEN_DIR})
-    add_custom_command(TARGET OffloadGenerate POST_BUILD COMMAND ${CMAKE_COMMAND}
-        -E copy_if_different OffloadErrcodes.inc "${LIBOMPTARGET_INCLUDE_DIR}/Shared/OffloadErrcodes.inc")


This seems to be missing after the change. @RossBrunton I'm not sure why it's separate from the others, is it because the output directory is different?

Yeah, this confused me because there's no such file in this directory and there's already a .inc file there. So I'm pretty sure this does nothing so I removed it.

@RossBrunton it's safe to delete this, right?

This is here because Shared/OffloadErrcodes.inc is in a different directory, yes. I still think it's required though.

Okay, so this is generated like the others but installed there. We really should just tablegen these on demand.

I very nearly have a PR ready to do that, should be up later today

Summary: Currently these are done POST_BUILD, but they should be dependencies on generating the library since it's a direct dependency.

Meinersbur · 2025-06-03T14:50:57Z

offload/liboffload/API/CMakeLists.txt

@@ -2,8 +2,8 @@
 # include directory. These files are checked in with the rest of the source,
 # therefore it is only needed when making changes to the API.

-find_program(CLANG_FORMAT clang-format PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
-if (CLANG_FORMAT)
+find_program(clang_format clang-format PATHS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)


find_program creates an entry into the CMakeCache.txt. By convention, those are upper case.

Meinersbur · 2025-06-03T14:53:17Z

offload/liboffload/API/CMakeLists.txt

-    add_custom_command(TARGET OffloadGenerate POST_BUILD COMMAND ${CMAKE_COMMAND}
-        -E copy_if_different OffloadErrcodes.inc "${LIBOMPTARGET_INCLUDE_DIR}/Shared/OffloadErrcodes.inc")
+
+    add_custom_target(OffloadAPI DEPENDS OffloadGenerate)


Suggested change

add_custom_target(OffloadAPI DEPENDS OffloadGenerate)

add_custom_target(OffloadAPI)

add_dependencies(OffloadAPI OffloadGenerate)

DEPENDS is for file-level dependencies

Meinersbur · 2025-06-03T14:55:32Z

offload/liboffload/CMakeLists.txt

+                           ${CMAKE_CURRENT_BINARY_DIR}/../include
+                           ${CMAKE_CURRENT_SOURCE_DIR}/include
+                           ${CMAKE_CURRENT_SOURCE_DIR}/include/generated
+                           ${CMAKE_CURRENT_SOURCE_DIR}/../include
+                           ${CMAKE_CURRENT_SOURCE_DIR}/../plugins-nextgen/common/include)


[nit] unrelated noise

jhuber6 requested review from jdoerfert, jplehr, Meinersbur and shiltian May 27, 2025 22:05

llvmbot added the offload label May 27, 2025

callumfare reviewed May 28, 2025

View reviewed changes

jhuber6 force-pushed the FixDependency branch from 6e56c3d to 091d45f Compare May 29, 2025 15:36

[Offload] Correctly regenerate API files if modified

e4e47de

Summary: Currently these are done POST_BUILD, but they should be dependencies on generating the library since it's a direct dependency.

jhuber6 force-pushed the FixDependency branch from 091d45f to e4e47de Compare May 29, 2025 15:53

callumfare mentioned this pull request May 29, 2025

[Offload] Don't check in generated files #141982

Merged

Meinersbur reviewed Jun 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Offload] Correctly regenerate API files if modified #141679

[Offload] Correctly regenerate API files if modified #141679

Uh oh!

jhuber6 commented May 27, 2025

Uh oh!

llvmbot commented May 27, 2025

Uh oh!

jhuber6 commented May 27, 2025

Uh oh!

callumfare left a comment

Uh oh!

callumfare May 28, 2025

Uh oh!

jhuber6 May 28, 2025

Uh oh!

jhuber6 May 29, 2025

Uh oh!

RossBrunton May 29, 2025

Uh oh!

jhuber6 May 29, 2025

Uh oh!

callumfare May 29, 2025

Uh oh!

jhuber6 May 29, 2025

Uh oh!

Meinersbur Jun 3, 2025

Uh oh!

Meinersbur Jun 3, 2025

Uh oh!

Meinersbur Jun 3, 2025

Uh oh!

Uh oh!

	add_custom_target(OffloadAPI DEPENDS OffloadGenerate)
	add_custom_target(OffloadAPI)
	add_dependencies(OffloadAPI OffloadGenerate)

[Offload] Correctly regenerate API files if modified #141679

Are you sure you want to change the base?

[Offload] Correctly regenerate API files if modified #141679

Uh oh!

Conversation

jhuber6 commented May 27, 2025

Uh oh!

llvmbot commented May 27, 2025

Uh oh!

jhuber6 commented May 27, 2025

Uh oh!

callumfare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!