[mlir][sparse] Fix breakage on older versions of cmake

[wrengr]

Summary

Per https://reviews.llvm.org/D136005#3866692 the introduction of the MLIRSparseTensorEnums target in D136002 caused breakage on some versions of cmake. This differential aims to fix those errors.

[aartbik]

mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt

22

please put a a bit more comment on why we need a phony target

[wrengr]

mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt

22

The conditional and phony target are just more copypasta from add_mlir_library. I'm not sure if they're actually needed/helpful here, but added them just in case. (Afaict, the conditional in add_mlir_library is to prevent add_mlir_library_install causing additional errors whenever the underlying call to add_llvm_library fails. But since I'm using add_library directly rather than going through add_llvm_library, I don't know if that's actually a concern here or not.)

The real/main change of this cl is just splitting the target_sources call out from the add_library call. But I'll add some more commentary to explain things.

[wrengr]

Adding comments

[wrengr]

Removing the CXX_STANDARD property

[wrengr]

In D136005#3868348, @psoni2628 wrote:

I am using CMake 3.18.0, so it's not extremely old.

After applying D136217, there is a different error message now, shown below.

-- LLVM host triple: x86_64-unknown-linux-gnu
-- LLVM default target triple: x86_64-unknown-linux-gnu
-- Building with -fPIC
-- Targeting X86
-- Clang version: 16.0.0
-- Not building amdgpu-arch: hsa-runtime64 not found
CMake Error at llvm-project/mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt:24 (set_property):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "CXX_STANDARD" is not allowed.

Maybe I should just upgrade my version of CMake. Which version of CMake are you using where it works?

I can't seem to find which version the phabricator buildbot is using, but locally I'm using 3.24.2. However, the LLVM project is supposed to work for any version >=3.13.4 (https://llvm.org/docs/GettingStarted.html#software), so it's definitely something I need to fix on our end.

I'm not sure which version changed things to allow setting the CXX_STANDARD property for INTERFACE libraries, so I'll just remove it for now and leave a comment about why. Can you try the new version of this differential?

[wrengr]

Tightening the version numbers in the comment about splitting add_library and target_sources

[uweigand]

This fixed the s390x build bot again. Thanks!

[DavidSpickett]

This fixed our bots. You might have gotten some failure mails from us but that was a networking issue, the builds are are green now. Thanks!

[psoni2628]

In D136217#3869290, @wrengr wrote:

In D136005#3868348, @psoni2628 wrote:

I am using CMake 3.18.0, so it's not extremely old.

After applying D136217, there is a different error message now, shown below.

-- LLVM host triple: x86_64-unknown-linux-gnu
-- LLVM default target triple: x86_64-unknown-linux-gnu
-- Building with -fPIC
-- Targeting X86
-- Clang version: 16.0.0
-- Not building amdgpu-arch: hsa-runtime64 not found
CMake Error at llvm-project/mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt:24 (set_property):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "CXX_STANDARD" is not allowed.

Maybe I should just upgrade my version of CMake. Which version of CMake are you using where it works?

I can't seem to find which version the phabricator buildbot is using, but locally I'm using 3.24.2. However, the LLVM project is supposed to work for any version >=3.13.4 (https://llvm.org/docs/GettingStarted.html#software), so it's definitely something I need to fix on our end.

I'm not sure which version changed things to allow setting the CXX_STANDARD property for INTERFACE libraries, so I'll just remove it for now and leave a comment about why. Can you try the new version of this differential?

Works for me too now, thanks!

mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt

 # This library is shared by both MLIRSparseTensorDialect and
 # MLIRSparseTensorRuntime, so it must not depend on any of the MLIR/LLVM
 # internals or else mlir_c_runner_utils will inherit that dependency.
 #
 # Because this is a header-only (`INTERFACE`) library, we cannot use
 # the `add_mlir_library` function.  So we do our best to replicate the
 # relevant portions below.  If doing so becomes too complicated, then
 # we should adjust the `add_mlir_library` function to also work for
 # `INTERFACE` libraries.
 set(MLIRSparseTensorEnums_srcs
     ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SparseTensor/IR/Enums.h)
-add_library(MLIRSparseTensorEnums INTERFACE ${MLIRSparseTensorEnums_srcs})
+# This conditional is copypasta from `add_mlir_library`.
 if(MSVC_IDE OR XCODE)
   set_source_files_properties(${MLIRSparseTensorEnums_srcs}
     PROPERTIES HEADER_FILE_ONLY ON)
 endif()
+# Older versions of cmake require INTERFACE libraries to separate the
+# `add_library` and `target_sources` calls.
+add_library(MLIRSparseTensorEnums INTERFACE)
+target_sources(MLIRSparseTensorEnums INTERFACE ${MLIRSparseTensorEnums_srcs})
+# The `add_mlir_library_install` is required for other libraries to
+# depend on this one, but the conditional itself and the phony target
+# are copypasta from `add_mlir_library`.  Afaict (wrengr), the version
+# in `add_mlir_library` is to prevent `add_mlir_library_install` from
+# raising additional errors whenever the underlying call to `add_llvm_library`
+# fails.  However, since we are using `add_library` directly, I'm not
+# sure if the conditional is helpful/required here or not.
+if(TARGET MLIRSparseTensorEnums)
   add_mlir_library_install(MLIRSparseTensorEnums)
+else()
+  # Add empty "phony" target
+  add_custom_target(MLIRSparseTensorEnums)
+endif()
+# Make sure this library adheres to the same CXX_STANDARD as mlir_c_runner_utils
 set_property(TARGET MLIRSparseTensorEnums PROPERTY CXX_STANDARD 17)
 
 
 add_mlir_dialect_library(MLIRSparseTensorDialect
   SparseTensorDialect.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SparseTensor