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

[wrengr]

Summary

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

Depends On D136002

[wrengr]

rebase

[wrengr]

git-clang-format

[wrengr]

Got CMake to work on linux

[aartbik]

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

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....

[wrengr]

mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h

31

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]

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

[wrengr]

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

[wrengr]

rebase

[wrengr]

rebase

[psoni2628]

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?

[wrengr]

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.

[wrengr]

Hi @psoni2628,

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

[psoni2628]

In D136005#3867045, @wrengr wrote:

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?

mlir/include/mlir/Dialect/SparseTensor/IR/Enums.h

 //===- Enums.h - Enums shared with the runtime ------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// Typedefs and enums for the lightweight runtime support library for
-// sparse tensor manipulations.  These are required to be public so that
-// they can be shared with `Transforms/SparseTensorConversion.cpp`, since
-// they define the arguments to the public functions declared later on.
+// Typedefs and enums shared between MLIR code for manipulating the
+// IR, and the lightweight runtime support library for sparse tensor
+// manipulations.  That is, all the enums are used to define the API
+// of the runtime library and hence are also needed when generating
+// calls into the runtime library.  Moveover, the `DimLevelType` enum
+// is also used as the internal IR encoding of dimension level types,
+// to avoid code duplication (e.g., for the predicates).
 //
 // This file also defines x-macros <https://en.wikipedia.org/wiki/X_Macro>
 // so that we can generate variations of the public functions for each
 // supported primary- and/or overhead-type.
 //
-// This file is part of the lightweight runtime support library for sparse
-// tensor manipulations.  The functionality of the support library is meant
-// to simplify benchmarking, testing, and debugging MLIR code operating on
-// sparse tensors.  However, the provided functionality is **not** part of
-// core MLIR itself.
+// Because this file defines a library which is a dependency of the
+// runtime library itself, this file must not depend on any MLIR internals
+// (e.g., operators, attributes, ArrayRefs, etc) lest the runtime library
+// inherit those dependencies.
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef MLIR_EXECUTIONENGINE_SPARSETENSOR_ENUMS_H
-#define MLIR_EXECUTIONENGINE_SPARSETENSOR_ENUMS_H
+#ifndef MLIR_DIALECT_SPARSETENSOR_IR_ENUMS_H
+#define MLIR_DIALECT_SPARSETENSOR_IR_ENUMS_H
 
 #include "mlir/ExecutionEngine/Float16bits.h"
 
 #include <cinttypes>
 #include <complex>
 
 namespace mlir {
 namespace sparse_tensor {
 
 /// This type is used in the public API at all places where MLIR expects
enum class Action : uint32_t {
   kFromFile = 1,
   kFromCOO = 2,
   kSparseToSparse = 3,
   kEmptyCOO = 4,
   kToCOO = 5,
   kToIterator = 6,
 };
 
-/// This enum mimics `SparseTensorEncodingAttr::DimLevelType` for
-/// breaking dependency cycles.  `SparseTensorEncodingAttr::DimLevelType`
-/// is the source of truth and this enum should be kept consistent with it.
+/// This enum defines all the sparse representations supportable by
+/// the SparseTensor dialect.  We use a lightweight encoding to encode
+/// both the "format" per se (dense, compressed, singleton) as well as
+/// the "properties" (ordered, unique).  The encoding is chosen for
+/// performance of the runtime library, and thus may change in future
+/// versions; consequently, client code should use the predicate functions
+/// defined below, rather than relying on knowledge about the particular
+/// binary encoding.
 enum class DimLevelType : uint8_t {
   Dense = 4,           // 0b001_00
   Compressed = 8,      // 0b010_00
   CompressedNu = 9,    // 0b010_01
   CompressedNo = 10,   // 0b010_10
   CompressedNuNo = 11, // 0b010_11
   Singleton = 16,      // 0b100_00
   SingletonNu = 17,    // 0b100_01
static_assert((isUniqueDLT(DimLevelType::Dense) &&
                !isUniqueDLT(DimLevelType::SingletonNu) &&
                isUniqueDLT(DimLevelType::SingletonNo) &&
                !isUniqueDLT(DimLevelType::SingletonNuNo)),
               "isUniqueDLT definition is broken");
 
 } // namespace sparse_tensor
 } // namespace mlir
 
-#endif // MLIR_EXECUTIONENGINE_SPARSETENSOR_ENUMS_H
+#endif // MLIR_DIALECT_SPARSETENSOR_IR_ENUMS_H

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h

 //===- SparseTensor.h - Sparse tensor dialect -------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_DIALECT_SPARSETENSOR_IR_SPARSETENSOR_H_
 #define MLIR_DIALECT_SPARSETENSOR_IR_SPARSETENSOR_H_
 
-#include "mlir/ExecutionEngine/SparseTensor/Enums.h"
+#include "mlir/Dialect/SparseTensor/IR/Enums.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/OpDefinition.h"
 #include "mlir/IR/OpImplementation.h"
 #include "mlir/IR/TensorEncoding.h"
 #include "mlir/Interfaces/InferTypeOpInterface.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 

mlir/include/mlir/ExecutionEngine/SparseTensor/Storage.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_EXECUTIONENGINE_SPARSETENSOR_STORAGE_H
 #define MLIR_EXECUTIONENGINE_SPARSETENSOR_STORAGE_H
 
 #include "mlir/ExecutionEngine/SparseTensor/COO.h"
 #include "mlir/ExecutionEngine/SparseTensor/CheckedMul.h"
-#include "mlir/ExecutionEngine/SparseTensor/Enums.h"
+#include "mlir/Dialect/SparseTensor/IR/Enums.h"
 #include "mlir/ExecutionEngine/SparseTensor/ErrorHandling.h"
 
 namespace mlir {
 namespace sparse_tensor {
 
 //===----------------------------------------------------------------------===//
 // This forward decl is sufficient to split `SparseTensorStorageBase` into
 // its own header, but isn't sufficient for `SparseTensorStorage` to join it.

mlir/include/mlir/ExecutionEngine/SparseTensorRuntime.h

 // support library for the SparseTensor dialect.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_EXECUTIONENGINE_SPARSETENSORRUNTIME_H
 #define MLIR_EXECUTIONENGINE_SPARSETENSORRUNTIME_H
 
 #include "mlir/ExecutionEngine/CRunnerUtils.h"
-#include "mlir/ExecutionEngine/SparseTensor/Enums.h"
+#include "mlir/Dialect/SparseTensor/IR/Enums.h"
 
 #include <cinttypes>
 #include <complex>
 #include <vector>
 
 using namespace mlir::sparse_tensor;
 
 extern "C" {

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.
+add_mlir_library(MLIRSparseTensorEnums
+  # TODO: how to list the single header in this library?
+  ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SparseTensor/IR/Enums.h
+
+  EXCLUDE_FROM_LIBMLIR
+
+  LINK_LIBS PUBLIC
+  mlir_float16_utils
+  )
+set_property(TARGET MLIRSparseTensorRuntime PROPERTY CXX_STANDARD 17)
+
 add_mlir_dialect_library(MLIRSparseTensorDialect
   SparseTensorDialect.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SparseTensor
 
   DEPENDS
   MLIRSparseTensorAttrDefsIncGen

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h

 #define MLIR_DIALECT_SPARSETENSOR_TRANSFORMS_CODEGENUTILS_H_
 
 #include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/Complex/IR/Complex.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/Dialect/SparseTensor/IR/SparseTensor.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
-#include "mlir/ExecutionEngine/SparseTensor/Enums.h"
+#include "mlir/Dialect/SparseTensor/IR/Enums.h"
 #include "mlir/IR/Builders.h"
 
 namespace mlir {
 
 class Location;
 class Type;
 class Value;
 

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

 #include "mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h"
 #include "mlir/Dialect/Bufferization/IR/Bufferization.h"
 #include "mlir/Dialect/Linalg/Utils/Utils.h"
 #include "mlir/Dialect/MemRef/IR/MemRef.h"
 #include "mlir/Dialect/SCF/IR/SCF.h"
 #include "mlir/Dialect/SparseTensor/IR/SparseTensor.h"
 #include "mlir/Dialect/SparseTensor/Transforms/Passes.h"
 #include "mlir/Dialect/Tensor/IR/Tensor.h"
-#include "mlir/ExecutionEngine/SparseTensor/Enums.h"
+#include "mlir/Dialect/SparseTensor/IR/Enums.h"
 #include "mlir/Transforms/DialectConversion.h"
 
 using namespace mlir;
 using namespace mlir::sparse_tensor;
 
 namespace {
 
 //===----------------------------------------------------------------------===//

mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt

-# This library is shared by both SparseTensorDialect and
-# SparseTensorRuntime, so it must not depend on any of the MLIR/LLVM
-# internals or else mlir_c_runner_utils will inherit that dependency.
-add_mlir_library(MLIRSparseTensorEnums
-  # TODO: how to list the single header in this library?
-  ${MLIR_MAIN_INCLUDE_DIR}/mlir/ExecutionEngine/SparseTensor/Enums.h
-
-  EXCLUDE_FROM_LIBMLIR
-
-  LINK_LIBS PUBLIC
-  mlir_float16_utils
-  )
-set_property(TARGET MLIRSparseTensorRuntime PROPERTY CXX_STANDARD 17)
-
 # Unlike mlir_float16_utils, mlir_c_runner_utils, etc, we do *not* make
 # this a shared library: because doing so causes issues when building
 # on Windows.  In particular, various functions take/return `std::vector`
 # but that class is not designed with dllimport/dllexport pragma,
 # therefore it cannot be safely/correctly used across DLL boundaries.
 # Consequently, we avoid using the "mlir_xxx_utils" naming scheme, since
 # that is reserved/intended for shared libraries only.
 add_mlir_library(MLIRSparseTensorRuntime

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel

 )
 
 # This library is shared by both SparseTensorDialect and
 # SparseTensorRuntime, so it must not depend on any of the MLIR/LLVM
 # internals or else mlir_c_runner_utils will inherit that dependency.
 cc_library(
     name = "SparseTensorEnums",
     srcs = [],
-    hdrs = ["include/mlir/ExecutionEngine/SparseTensor/Enums.h"],
+    hdrs = ["include/mlir/Dialect/SparseTensor/IR/Enums.h"],
     includes = ["include"],
     deps = [":mlir_float16_utils"],
 )
 
 cc_library(
     name = "SparseTensorDialect",
     srcs = ["lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp"],
     hdrs = ["include/mlir/Dialect/SparseTensor/IR/SparseTensor.h"],