[mlir][sparse] refactoring SparseTensorUtils: (4 of 4) documentation

[wrengr]

Summary

Previously, the SparseTensorUtils.cpp library contained a C++ core implementation, but hid it in an anonymous namespace and only exposed a C-API for accessing it. Now we are factoring out that C++ core into a standalone C++ library so that it can be used directly by downstream clients (per request of one such client). This refactoring has been decomposed into a stack of differentials in order to simplify the code review process, however the full stack of changes should be considered together.

• D133462: Part 1: split one file into several
• D133830: Part 2: Reorder chunks within files
• D133831: Part 3: General code cleanup
• (this): Part 4: Update documentation

This part updates existing documentation, adds new documentation, and reflows the test for some existing documentation.

Depends On D133831

[wrengr]

rebase

[aartbik]

mlir/include/mlir/ExecutionEngine/SparseTensor/COO.h

98

We have Get and Adds now as style. I slightly prefer the active "s" form, but either way, let's pick one consistently.

[wrengr]

Rebasing to parent D133462 at Diff 463979.

[wrengr]

rebase

[wrengr]

mlir/include/mlir/ExecutionEngine/SparseTensor/COO.h

98

See D134986

mlir/include/mlir/ExecutionEngine/SparseTensor/COO.h

 #include <cassert>
 #include <cinttypes>
 #include <functional>
 #include <vector>
 
 namespace mlir {
 namespace sparse_tensor {
 
-/// A sparse tensor element in coordinate scheme (value and indices).
-/// For example, a rank-1 vector element would look like
+/// An element of a sparse tensor in coordinate-scheme representation
+/// (i.e., a pair of indices and value).  For example, a rank-1 vector
+/// element would look like
 ///   ({i}, a[i])
-/// and a rank-5 tensor element like
+/// and a rank-5 tensor element would look like
 ///   ({i,j,k,l,m}, a[i,j,k,l,m])
-/// We use pointer to a shared index pool rather than e.g. a direct
-/// vector since that (1) reduces the per-element memory footprint, and
-/// (2) centralizes the memory reservation and (re)allocation to one place.
+///
+/// The indices are represented as a (non-owning) pointer into a shared
+/// pool of indices, rather than being stored directly in this object.
+/// This significantly improves performance because it: (1) reduces
+/// the per-element memory footprint, and (2) centralizes the memory
+/// management for indices.  The only downside is that the indices
+/// themselves cannot be retrieved without knowing the rank of the
+/// tensor to which this element belongs (and that rank is not stored
+/// in this object).
 template <typename V>
 struct Element final {
   Element(const uint64_t *ind, V val) : indices(ind), value(val){};
   const uint64_t *indices; // pointer into shared index pool
   V value;
 };
 
 /// The type of callback functions which receive an element.  We avoid
 /// packaging the coordinates and value together as an `Element` object
 /// because this helps keep code somewhat cleaner.
 template <typename V>
 using ElementConsumer =
     const std::function<void(const std::vector<uint64_t> &, V)> &;
 
-/// A memory-resident sparse tensor in coordinate scheme (collection of
-/// elements). This data structure is used to read a sparse tensor from
-/// any external format into memory and sort the elements lexicographically
-/// by indices before passing it back to the client (most packed storage
-/// formats require the elements to appear in lexicographic index order).
+/// A memory-resident sparse tensor in coordinate-scheme representation
+/// (a collection of `Element`s).  This data structure is used as
+/// an intermediate representation; e.g., for reading sparse tensors
+/// from external formats into memory, or for certain conversions between
+/// different `SparseTensorStorage` formats.
 template <typename V>
 class SparseTensorCOO final {
 public:
   SparseTensorCOO(const std::vector<uint64_t> &dimSizes, uint64_t capacity)
       : dimSizes(dimSizes) {
     if (capacity) {
       elements.reserve(capacity);
       indices.reserve(capacity * getRank());
     }
   }
 
   /// Factory method. Permutes the original dimensions according to
   /// the given ordering and expects subsequent add() calls to honor
   /// that same ordering for the given indices. The result is a
   /// fully permuted coordinate scheme.
   ///
   /// Precondition: `dimSizes` and `perm` must be valid for `rank`.
+  ///
+  /// Asserts: the elements of `dimSizes` are non-zero.
   static SparseTensorCOO<V> *newSparseTensorCOO(uint64_t rank,
                                                 const uint64_t *dimSizes,
                                                 const uint64_t *perm,
                                                 uint64_t capacity = 0) {
     std::vector<uint64_t> permsz(rank);
     for (uint64_t r = 0; r < rank; ++r) {
       assert(dimSizes[r] > 0 && "Dimension size zero has trivial storage");
       permsz[perm[r]] = dimSizes[r];
     }
     return new SparseTensorCOO<V>(permsz, capacity);
   }
 
   /// Get the rank of the tensor.
   uint64_t getRank() const { return dimSizes.size(); }
 
-  /// Getter for the dimension-sizes array.
+  /// Get the dimension-sizes array.
   const std::vector<uint64_t> &getDimSizes() const { return dimSizes; }
 
-  /// Getter for the elements array.
+  /// Get the elements array.
   const std::vector<Element<V>> &getElements() const { return elements; }
 
-  /// Adds element as indices and value.
+  /// Adds an element to the tensor.  This method does not check whether
+  /// `ind` is already associated with a value, it adds it regardless.
+  /// Resolving such conflicts is left up to clients of the iterator
+  /// interface.
+  ///
+  /// Asserts:
+  /// * is not in iterator mode
+  /// * the `ind` is valid for `rank`
+  /// * the elements of `ind` are valid for `dimSizes`.
   void add(const std::vector<uint64_t> &ind, V val) {
     assert(!iteratorLocked && "Attempt to add() after startIterator()");
     const uint64_t *base = indices.data();
     uint64_t size = indices.size();
     uint64_t rank = getRank();
     assert(ind.size() == rank && "Element rank mismatch");
     for (uint64_t r = 0; r < rank; ++r) {
       assert(ind[r] < dimSizes[r] && "Index is too large for the dimension");
if (newBase != base) {
       for (uint64_t i = 0, n = elements.size(); i < n; ++i)
         elements[i].indices = newBase + (elements[i].indices - base);
       base = newBase;
     }
     // Add element as (pointer into shared index pool, value) pair.
     elements.emplace_back(base + size, val);
   }
 
-  /// Sorts elements lexicographically by index.
+  /// Sorts elements lexicographically by index.  If an index is mapped to
+  /// multiple values, then the relative order of those values is unspecified.
+  ///
+  /// Asserts: is not in iterator mode.
   void sort() {
     assert(!iteratorLocked && "Attempt to sort() after startIterator()");
     // TODO: we may want to cache an `isSorted` bit, to avoid
     // unnecessary/redundant sorting.
     uint64_t rank = getRank();
     std::sort(elements.begin(), elements.end(),
               [rank](const Element<V> &e1, const Element<V> &e2) {
                 for (uint64_t r = 0; r < rank; ++r) {
                   if (e1.indices[r] == e2.indices[r])
                     continue;
                   return e1.indices[r] < e2.indices[r];
                 }
                 return false;
               });
   }
 
-  /// Switch into iterator mode.
+  /// Switch into iterator mode.  If already in iterator mode, then
+  /// resets the position to the first element.
   void startIterator() {
     iteratorLocked = true;
     iteratorPos = 0;
   }
 
-  /// Get the next element.
+  /// Get the next element.  If there are no remaining elements, then
+  /// returns nullptr and switches out of iterator mode.
+  ///
+  /// Asserts: is in iterator mode.
   const Element<V> *getNext() {
     assert(iteratorLocked && "Attempt to getNext() before startIterator()");
     if (iteratorPos < elements.size())
       return &(elements[iteratorPos++]);
     iteratorLocked = false;
     return nullptr;
   }
 

mlir/include/mlir/ExecutionEngine/SparseTensor/CheckedMul.h

 #include <cassert>
 #include <cinttypes>
 #include <limits>
 
 namespace mlir {
 namespace sparse_tensor {
 namespace detail {
 
-/// A version of `operator*` on `uint64_t` which checks for overflows.
+// TODO: would be better to use various architectures' intrinsics to
+// detect the overflow directly, instead of doing the assertion beforehand
+// (which requires an expensive division).
+//
+/// A version of `operator*` on `uint64_t` which guards against overflows
+/// (when assertions are enabled).
 inline uint64_t checkedMul(uint64_t lhs, uint64_t rhs) {
   assert((lhs == 0 || rhs <= std::numeric_limits<uint64_t>::max() / lhs) &&
          "Integer overflow");
   return lhs * rhs;
 }
 
 } // namespace detail
 } // namespace sparse_tensor
 } // namespace mlir
 
 #endif // MLIR_EXECUTIONENGINE_SPARSETENSOR_CHECKEDMUL_H

mlir/include/mlir/ExecutionEngine/SparseTensor/ErrorHandling.h

 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_EXECUTIONENGINE_SPARSETENSOR_ERRORHANDLING_H
 #define MLIR_EXECUTIONENGINE_SPARSETENSOR_ERRORHANDLING_H
 
 #include <cstdio>
 #include <cstdlib>
 
-// This macro helps minimize repetition of this idiom, as well as ensuring
-// we have some additional output indicating where the error is coming from.
-// (Since `fprintf` doesn't provide a stacktrace, this helps make it easier
-// to track down whether an error is coming from our code vs somewhere else
-// in MLIR.)
+/// This macro helps minimize repetition of the printf-and-exit idiom,
+/// as well as ensuring that we print some additional output indicating
+/// where the error is coming from--- to make it easier to determine
+/// whether some particular error is coming from the runtime library's
+/// code vs from somewhere else in the MLIR stack.  (Since that can be
+/// hard to determine without the stacktraces provided by assertion failures.)
 #define MLIR_SPARSETENSOR_FATAL(...)                                           \
   do {                                                                         \
     fprintf(stderr, "SparseTensorUtils: " __VA_ARGS__);                        \
     exit(1);                                                                   \
   } while (0)
 
 #endif // MLIR_EXECUTIONENGINE_SPARSETENSOR_ERRORHANDLING_H

mlir/include/mlir/ExecutionEngine/SparseTensor/File.h

 // after reading the header; so as to statically avoid the need
 // to `assert(isValid())`.
 
 /// This class abstracts over the information stored in file headers,
 /// as well as providing the buffers and methods for parsing those headers.
 class SparseTensorFile final {
 public:
   enum class ValueKind {
+    // The value before calling `readHeader`.
     kInvalid = 0,
+    // Values that can be set by `readMMEHeader`.
     kPattern = 1,
     kReal = 2,
     kInteger = 3,
     kComplex = 4,
+    // The value set by `readExtFROSTTHeader`.
     kUndefined = 5
   };
 
   explicit SparseTensorFile(const char *filename) : filename(filename) {
     assert(filename && "Received nullptr for filename");
   }
 
   // Disallows copying, to avoid duplicating the `file` pointer.
public:
   /// Attempts to read a line from the file.
   char *readLine();
 
   /// Reads and parses the file's header.
   void readHeader();
 
   ValueKind getValueKind() const { return valueKind_; }
 
+  /// Checks if a header has been successfully read.
   bool isValid() const { return valueKind_ != ValueKind::kInvalid; }
 
   /// Gets the MME "pattern" property setting.  Is only valid after
   /// parsing the header.
   bool isPattern() const {
     assert(isValid() && "Attempt to isPattern() before readHeader()");
     return valueKind_ == ValueKind::kPattern;
   }
uint64_t getDimSize(uint64_t d) const {
     return idata[2 + d];
   }
 
   /// Asserts the shape subsumes the actual dimension sizes.  Is only
   /// valid after parsing the header.
   void assertMatchesShape(uint64_t rank, const uint64_t *shape) const;
 
 private:
+  /// Read the MME header of a general sparse matrix of type real.
   void readMMEHeader();
+
+  /// Read the "extended" FROSTT header. Although not part of the
+  /// documented format, we assume that the file starts with optional
+  /// comments followed by two lines that define the rank, the number of
+  /// nonzeros, and the dimensions sizes (one per rank) of the sparse tensor.
   void readExtFROSTTHeader();
 
   static constexpr int kColWidth = 1025;
   const char *const filename;
   FILE *file = nullptr;
   ValueKind valueKind_ = ValueKind::kInvalid;
   bool isSymmetric_ = false;
   uint64_t idata[512];
   char line[kColWidth];
 };
 
+//===----------------------------------------------------------------------===//
 namespace detail {
 
 // Adds a value to a tensor in coordinate scheme. If is_symmetric_value is true,
 // also adds the value to its symmetric location.
 template <typename T, typename V>
 inline void addValue(T *coo, V value, const std::vector<uint64_t> indices,
                      bool is_symmetric_value) {
   // TODO: <https://github.com/llvm/llvm-project/issues/54179>
   coo->add(indices, value);
   // We currently chose to deal with symmetric matrices by fully constructing
   // them. In the future, we may want to make symmetry implicit for storage
   // reasons.
   if (is_symmetric_value)
     coo->add({indices[1], indices[0]}, value);
 }
 
-// Reads an element of a complex type for the current indices in coordinate
-// scheme.
+/// Reads an element of a complex type for the current indices in
+/// coordinate scheme.
 template <typename V>
 inline void readCOOValue(SparseTensorCOO<std::complex<V>> *coo,
                          const std::vector<uint64_t> indices, char **linePtr,
                          bool is_pattern, bool add_symmetric_value) {
   // Read two values to make a complex. The external formats always store
   // numerical values with the type double, but we cast these values to the
   // sparse tensor object type. For a pattern tensor, we arbitrarily pick the
   // value 1 for all entries.

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

public:
   SparseTensorStorageBase(const std::vector<uint64_t> &dimSizes,
                           const uint64_t *perm, const DimLevelType *sparsity);
 
   virtual ~SparseTensorStorageBase() = default;
 
   /// Get the rank of the tensor.
   uint64_t getRank() const { return dimSizes.size(); }
 
-  /// Getter for the dimension-sizes array, in storage-order.
+  /// Get the dimension-sizes array, in storage-order.
   const std::vector<uint64_t> &getDimSizes() const { return dimSizes; }
 
   /// Safely lookup the size of the given (storage-order) dimension.
   uint64_t getDimSize(uint64_t d) const {
     ASSERT_VALID_DIM(d);
     return dimSizes[d];
   }
 
-  /// Getter for the "reverse" permutation, which maps this object's
+  /// Get the "reverse" permutation, which maps this object's
   /// storage-order to the tensor's semantic-order.
   const std::vector<uint64_t> &getRev() const { return rev; }
 
-  /// Getter for the dimension-types array, in storage-order.
+  /// Get the dimension-types array, in storage-order.
   const std::vector<DimLevelType> &getDimTypes() const { return dimTypes; }
 
   /// Safely check if the (storage-order) dimension uses dense storage.
   bool isDenseDim(uint64_t d) const {
     ASSERT_VALID_DIM(d);
     return dimTypes[d] == DimLevelType::kDense;
   }
 
public:
 
   /// Allocate a new enumerator.
 #define DECL_NEWENUMERATOR(VNAME, V)                                           \
   virtual void newEnumerator(SparseTensorEnumeratorBase<V> **, uint64_t,       \
                              const uint64_t *) const;
   FOREVERY_V(DECL_NEWENUMERATOR)
 #undef DECL_NEWENUMERATOR
 
-  /// Overhead storage.
+  /// Pointers-overhead storage.
 #define DECL_GETPOINTERS(PNAME, P)                                             \
   virtual void getPointers(std::vector<P> **, uint64_t);
   FOREVERY_FIXED_O(DECL_GETPOINTERS)
 #undef DECL_GETPOINTERS
+
+  /// Indices-overhead storage.
 #define DECL_GETINDICES(INAME, I)                                              \
   virtual void getIndices(std::vector<I> **, uint64_t);
   FOREVERY_FIXED_O(DECL_GETINDICES)
 #undef DECL_GETINDICES
 
   /// Primary storage.
 #define DECL_GETVALUES(VNAME, V) virtual void getValues(std::vector<V> **);
   FOREVERY_V(DECL_GETVALUES)

 
 //===----------------------------------------------------------------------===//
 // This forward decl is necessary for defining `SparseTensorStorage`,
 // but isn't sufficient for splitting it off.
 template <typename P, typename I, typename V>
 class SparseTensorEnumerator;
 
 /// A memory-resident sparse tensor using a storage scheme based on
-/// per-dimension sparse/dense annotations. This data structure provides a
-/// bufferized form of a sparse tensor type. In contrast to generating setup
-/// methods for each differently annotated sparse tensor, this method provides
-/// a convenient "one-size-fits-all" solution that simply takes an input tensor
-/// and annotations to implement all required setup in a general manner.
+/// per-dimension sparse/dense annotations.  This data structure provides
+/// a bufferized form of a sparse tensor type.  In contrast to generating
+/// setup methods for each differently annotated sparse tensor, this
+/// method provides a convenient "one-size-fits-all" solution that simply
+/// takes an input tensor and annotations to implement all required setup
+/// in a general manner.
 template <typename P, typename I, typename V>
 class SparseTensorStorage final : public SparseTensorStorageBase {
   /// Private constructor to share code between the other constructors.
   /// Beware that the object is not necessarily guaranteed to be in a
   /// valid state after this constructor alone; e.g., `isCompressedDim(d)`
   /// doesn't entail `!(pointers[d].empty())`.
   ///
   /// Precondition: `perm` and `sparsity` must be valid for `dimSizes.size()`.
public:
   /// Finalizes lexicographic insertions.
   void endInsert() final {
     if (values.empty())
       finalizeSegment(0);
     else
       endPath(0);
   }
 
+  /// Allocate a new enumerator for this classes `<P,I,V>` types and
+  /// erase the `<P,I>` parts from the type.  Callers must make sure to
+  /// delete the enumerator when they're done with it.
   void newEnumerator(SparseTensorEnumeratorBase<V> **out, uint64_t rank,
                      const uint64_t *perm) const final {
     *out = new SparseTensorEnumerator<P, I, V>(*this, rank, perm);
   }
 
   /// Returns this sparse tensor storage scheme as a new memory-resident
   /// sparse tensor in coordinate scheme with the given dimension order.
   ///

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

 void SparseTensorFile::assertMatchesShape(uint64_t rank,
                                           const uint64_t *shape) const {
   assert(rank == getRank() && "Rank mismatch");
   for (uint64_t r = 0; r < rank; ++r)
     assert((shape[r] == 0 || shape[r] == idata[2 + r]) &&
            "Dimension size mismatch");
 }
 
-/// Helper to convert string to lower case.
+/// Helper to convert C-style strings (i.e., '\0' terminated) to lower case.
 static inline char *toLower(char *token) {
   for (char *c = token; *c; ++c)
     *c = tolower(*c);
   return token;
 }
 
 /// Read the MME header of a general sparse matrix of type real.
 void SparseTensorFile::readMMEHeader() {

mlir/lib/ExecutionEngine/SparseTensor/NNZ.cpp

 // core MLIR itself.
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/ExecutionEngine/SparseTensor/Storage.h"
 
 using namespace mlir::sparse_tensor;
 
+//===----------------------------------------------------------------------===//
+/// Allocate the statistics structure for the desired sizes and
+/// sparsity (in the target tensor's storage-order).  This constructor
+/// does not actually populate the statistics, however; for that see
+/// `initialize`.
+///
+/// Precondition: `dimSizes` must not contain zeros.
 SparseTensorNNZ::SparseTensorNNZ(const std::vector<uint64_t> &dimSizes,
                                  const std::vector<DimLevelType> &sparsity)
     : dimSizes(dimSizes), dimTypes(sparsity), nnz(getRank()) {
   assert(dimSizes.size() == dimTypes.size() && "Rank mismatch");
   bool uncompressed = true;
   (void)uncompressed;
   uint64_t sz = 1; // the product of all `dimSizes` strictly less than `r`.
   for (uint64_t rank = getRank(), r = 0; r < rank; r++) {
case DimLevelType::kSingleton:
       break;
     default:
       MLIR_SPARSETENSOR_FATAL("unsupported dimension level type");
     }
     sz = detail::checkedMul(sz, dimSizes[r]);
   }
 }
 
+/// Lexicographically enumerates all indicies for dimensions strictly
+/// less than `stopDim`, and passes their nnz statistic to the callback.
+/// Since our use-case only requires the statistic not the coordinates
+/// themselves, we do not bother to construct those coordinates.
 void SparseTensorNNZ::forallIndices(uint64_t stopDim,
                                     SparseTensorNNZ::NNZConsumer yield) const {
   assert(stopDim < getRank() && "Dimension out of bounds");
   assert(dimTypes[stopDim] == DimLevelType::kCompressed &&
          "Cannot look up non-compressed dimensions");
   forallIndices(yield, stopDim, 0, 0);
 }
 
+/// Adds a new element (i.e., increment its statistics).  We use
+/// a method rather than inlining into the lambda in `initialize`,
+/// to avoid spurious templating over `V`.  And this method is private
+/// to avoid needing to re-assert validity of `ind` (which is guaranteed
+/// by `forallElements`).
 void SparseTensorNNZ::add(const std::vector<uint64_t> &ind) {
   uint64_t parentPos = 0;
   for (uint64_t rank = getRank(), r = 0; r < rank; ++r) {
     if (dimTypes[r] == DimLevelType::kCompressed)
       nnz[r][parentPos]++;
     parentPos = parentPos * dimSizes[r] + ind[r];
   }
 }
 
+/// Recursive component of the public `forallIndices`.
 void SparseTensorNNZ::forallIndices(SparseTensorNNZ::NNZConsumer yield,
                                     uint64_t stopDim, uint64_t parentPos,
                                     uint64_t d) const {
   assert(d <= stopDim);
   if (d == stopDim) {
     assert(parentPos < nnz[d].size() && "Cursor is out of range");
     yield(nnz[d][parentPos]);
   } else {
     const uint64_t sz = dimSizes[d];
     const uint64_t pstart = parentPos * sz;
     for (uint64_t i = 0; i < sz; i++)
       forallIndices(yield, stopDim, pstart + i, d + 1);
   }
 }

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp

 //===- SparseTensorUtils.cpp - Sparse Tensor Utils for MLIR execution -----===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
-// This file implements a light-weight runtime support library that is useful
-// for sparse tensor manipulations. The functionality provided in this library
-// is meant to simplify benchmarking, testing, and debugging MLIR code that
-// operates on sparse tensors. The provided functionality is **not** part
-// of core MLIR, however.
+// This file implements a light-weight runtime support library for
+// manipulating sparse tensors from MLIR.  More specifically, it provides
+// C-API wrappers so that MLIR-generated code can call into the C++ runtime
+// support library.  The functionality provided in this library is meant
+// to simplify benchmarking, testing, and debugging of MLIR code operating
+// on sparse tensors.  However, the provided functionality is **not**
+// part of core MLIR itself.
 //
 // The following memory-resident sparse storage schemes are supported:
 //
 // (a) A coordinate scheme for temporarily storing and lexicographically
 //     sorting a sparse tensor by index (SparseTensorCOO).
 //
 // (b) A "one-size-fits-all" sparse tensor storage scheme defined by
 //     per-dimension sparse/dense annnotations together with a dimension

 #include "mlir/ExecutionEngine/SparseTensor/ErrorHandling.h"
 #include "mlir/ExecutionEngine/SparseTensor/File.h"
 #include "mlir/ExecutionEngine/SparseTensor/Storage.h"
 
 #include <numeric>
 
 using namespace mlir::sparse_tensor;
 
+//===----------------------------------------------------------------------===//
+//
+// Implementation details for public functions, which don't have a good
+// place to live in the C++ library this file is wrapping.
+//
+//===----------------------------------------------------------------------===//
+
 namespace {
 
 /// Initializes sparse tensor from an external COO-flavored format.
+/// Used by `IMPL_CONVERTTOMLIRSPARSETENSOR`.
+// TODO: generalize beyond 64-bit indices.
 template <typename V>
 static SparseTensorStorage<uint64_t, uint64_t, V> *
 toMLIRSparseTensor(uint64_t rank, uint64_t nse, const uint64_t *shape,
                    const V *values, const uint64_t *indices,
                    const uint64_t *perm, const DimLevelType *sparsity) {
 #ifndef NDEBUG
   // Verify that perm is a permutation of 0..(rank-1).
   std::vector<uint64_t> order(perm, perm + rank);
#endif
   // Return sparse tensor storage format as opaque pointer.
   auto *tensor = SparseTensorStorage<uint64_t, uint64_t, V>::newSparseTensor(
       rank, shape, perm, sparsity, coo);
   delete coo;
   return tensor;
 }
 
 /// Converts a sparse tensor to an external COO-flavored format.
+/// Used by `IMPL_CONVERTFROMMLIRSPARSETENSOR`.
+//
+// TODO: Currently, values are copied from SparseTensorStorage to
+// SparseTensorCOO, then to the output.  We may want to reduce the number
+// of copies.
+//
+// TODO: generalize beyond 64-bit indices, no dim ordering, all dimensions
+// compressed
 template <typename V>
 static void
 fromMLIRSparseTensor(const SparseTensorStorage<uint64_t, uint64_t, V> *tensor,
                      uint64_t *pRank, uint64_t *pNse, uint64_t **pShape,
                      V **pValues, uint64_t **pIndices) {
   assert(tensor && "Received nullptr for tensor");
   uint64_t rank = tensor->getRank();
   std::vector<uint64_t> perm(rank);
void readSparseTensorShape(char *filename, std::vector<uint64_t> *out) {
   stfile.closeFile();
   const uint64_t rank = stfile.getRank();
   const uint64_t *dimSizes = stfile.getDimSizes();
   out->reserve(rank);
   out->assign(dimSizes, dimSizes + rank);
 }
 
 // We can't use `static_cast` here because `DimLevelType` is an enum-class.
-// TODO: generalize beyond 64-bit indices.
 #define IMPL_CONVERTTOMLIRSPARSETENSOR(VNAME, V)                               \
   void *convertToMLIRSparseTensor##VNAME(                                      \
       uint64_t rank, uint64_t nse, uint64_t *shape, V *values,                 \
       uint64_t *indices, uint64_t *perm, uint8_t *sparse) {                    \
     return toMLIRSparseTensor<V>(rank, nse, shape, values, indices, perm,      \
                                  reinterpret_cast<DimLevelType *>(sparse));    \
   }
 FOREVERY_V(IMPL_CONVERTTOMLIRSPARSETENSOR)
 #undef IMPL_CONVERTTOMLIRSPARSETENSOR
 
-// TODO: Currently, values are copied from SparseTensorStorage to
-// SparseTensorCOO, then to the output.  We may want to reduce the number
-// of copies.
-//
-// TODO: generalize beyond 64-bit indices, no dim ordering, all dimensions
-// compressed
 #define IMPL_CONVERTFROMMLIRSPARSETENSOR(VNAME, V)                             \
   void convertFromMLIRSparseTensor##VNAME(void *tensor, uint64_t *pRank,       \
                                           uint64_t *pNse, uint64_t **pShape,   \
                                           V **pValues, uint64_t **pIndices) {  \
     fromMLIRSparseTensor<V>(                                                   \
         static_cast<SparseTensorStorage<uint64_t, uint64_t, V> *>(tensor),     \
         pRank, pNse, pShape, pValues, pIndices);                               \
   }
 FOREVERY_V(IMPL_CONVERTFROMMLIRSPARSETENSOR)
 #undef IMPL_CONVERTFROMMLIRSPARSETENSOR
 
 } // extern "C"
 
 #endif // MLIR_CRUNNERUTILS_DEFINE_FUNCTIONS