This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Moving Enums.h into Dialect/SparseTensor/IR
ClosedPublic

Authored by wrengr on Oct 14 2022, 5:28 PM.

Details

Summary

Move the SparseTensorEnums library out of the ExecutionEngine directory and into Dialect/SparseTensor/IR.

Depends On D136002

Diff Detail

Event Timeline

wrengr created this revision.Oct 14 2022, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 5:28 PM
wrengr requested review of this revision.Oct 14 2022, 5:28 PM
wrengr retitled this revision from [WIP] Moving Enums.h into Dialect/SparseTensor/IR to [mlir][sparse] Moving Enums.h into Dialect/SparseTensor/IR.Oct 14 2022, 5:35 PM
wrengr updated this revision to Diff 467988.Oct 14 2022, 6:00 PM

git-clang-format

wrengr updated this revision to Diff 468339.Oct 17 2022, 2:52 PM

Got CMake to work on linux

aartbik accepted this revision.Oct 17 2022, 5:20 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h
1

probably want to make this onliner no longer reference runtime, but just say "enums" in some manner

(you explain that well enought at L9 and down)

31–32

This is just needed for MLIR_SPARSETENSOR_FOREVERY_V right?

Does it work without this include if we push the include into the clients of this macro?
Right here, it is just "text"....

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

+1 for that by the way, I had the use for such an add macro in the past, if I remember correctly, and also hacked my way around it a bit....

This revision is now accepted and ready to land.Oct 17 2022, 5:20 PM
wrengr marked an inline comment as done.Oct 17 2022, 5:58 PM
wrengr added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h
31–32

Yeah, it's just needed for the MLIR_SPARSETENSOR_FOREVERY_V macro.

Everything compiles fine if I remove it (and add it to the appropriate other places). Removing it makes me a little squeamish, since it's poor hygiene. But since there're no actual checks on macro definitions, there's no actual benefit to including the header here anyways.

Since removing the header/dependency is more about the original formulation of the SparseTensorEnums library, I'll do the changes at D136002 instead of here. Since that helps keep the CLs a bit cleaner.

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

Do you remember where? After reading through the add_mlir_library definition I think it should be pretty easy to change; it's just a question of whether the upstream folks would push back or not

wrengr updated this revision to Diff 468385.Oct 17 2022, 6:29 PM
wrengr marked an inline comment as done.

rebasing for the change in D136002 (removing the mlir_float16_utils dependency)

wrengr updated this revision to Diff 468386.Oct 17 2022, 6:34 PM

correcting a rebase error in lib/Dialect/SparseTensor/IR/CMakeLists.txt

wrengr marked an inline comment as done.Oct 17 2022, 6:38 PM
This revision was automatically updated to reflect the committed changes.

I am getting the following CMake error below.

CMake Error at llvm-project/mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt:12 (add_library):
  add_library INTERFACE library requires no source arguments.


CMake Error at llvm-project/mlir/cmake/modules/AddMLIR.cmake:533 (install):
  install TARGETS given target "MLIRSparseTensorEnums" which does not exist.
Call Stack (most recent call first):
  llvm-project/mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt:17 (add_mlir_library_install)


CMake Error at llvm-project/mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt:18 (set_property):
  set_property could not find TARGET MLIRSparseTensorEnums.  Perhaps it has
  not yet been created.


-- Performing Test COMPILER_SUPPORTS_WARNING_WEAK_VTABLES
-- Performing Test COMPILER_SUPPORTS_WARNING_WEAK_VTABLES - Failed
CMake Error at llvm-project/mlir/cmake/modules/CMakeLists.txt:21 (export):
  export given target "MLIRSparseTensorEnums" which is not built by this
  project.

After reverting this patch and D136002, the errors go away. Any ideas what might be the problem?

Hi @psoni2628,

The differentials D135995, D135996, D136002, D136005, and D136123 form a related sequence of changes; and D136002 is the one that first added the MLIRSparseTensorEnums target.

It looks like your version of cmake differs from mine (and the phabricator buildbot's). So far as I can tell, the fix should be to update mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt of D136002 (and consequently mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt of D136005) to split the add_library line into two parts:

add_library(MLIRSparseTensorEnums INTERFACE)
target_sources(MLIRSparseTensorEnums INTERFACE ${MLIRSparseTensorEnums_srcs})

I can make a new differential to implement that change, but since I've already landed D136123 the obvious thing would be to update mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt; however that leaves things in a bad state from when D136002 landed until the new differential does. I'll post the new differential here in a couple minutes, but let me know if there's anything else you need to get things working on your end.

Hi @psoni2628,

I just posted D136217. Please comment there to let me know if it works for you.

Hi @psoni2628,

I just posted D136217. Please comment there to let me know if it works for you.

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?