This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by wrengr on Oct 18 2022, 6:17 PM.

Details

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.

Diff Detail

Event Timeline

wrengr created this revision.Oct 18 2022, 6:17 PM
wrengr requested review of this revision.Oct 18 2022, 6:17 PM
aartbik accepted this revision.Oct 19 2022, 8:49 AM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt
21

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

This revision is now accepted and ready to land.Oct 19 2022, 8:49 AM
wrengr marked an inline comment as done.Oct 19 2022, 12:54 PM
wrengr added inline comments.
mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt
21

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 updated this revision to Diff 469017.Oct 19 2022, 12:57 PM
wrengr marked an inline comment as done.

Adding comments

wrengr updated this revision to Diff 469022.Oct 19 2022, 1:17 PM

Removing the CXX_STANDARD property

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 updated this revision to Diff 469031.Oct 19 2022, 1:40 PM

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

This revision was landed with ongoing or failed builds.Oct 19 2022, 1:56 PM
This revision was automatically updated to reflect the committed changes.

This fixed the s390x build bot again. Thanks!

DavidSpickett added a comment.EditedOct 20 2022, 1:46 AM

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!

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!