[mlir][sparse] fix conversion bug when changing pointer/index sizes

[wrengr]

LGTM.

[wrengr]

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

593–596

(The lgtm notwithstanding), If we're so concerned to share the contents of params, then at this point I think newParams() should be refactored into smaller chunks so that we can share/hide all this boilerplate (and avoid the inevitable magic numbers issues re the indices into params)

[aartbik]

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

593–596

I agree! This fixes a subtle bug for the world, but I also was thinking of exactly this. A nice follow up refactoring perhaps? ;-)

[aartbik]

rebased with main

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

if (encDst && encSrc) {
       if (encDst == encSrc) {
         rewriter.replaceOp(op, adaptor.getOperands()); // hidden nop cast
         return success();
       }
       SmallVector<Value, 4> sizes;
       SmallVector<Value, 8> params;
       sizesFromPtr(rewriter, sizes, op, encSrc, srcType.cast<ShapedType>(),
                    src);
-      newParams(rewriter, params, op, encDst, kToCOO, sizes, src);
+      // Set up encoding with right mix of src and dst so that the two
+      // method calls can share most parameters, while still providing
+      // the correct sparsity information to either of them.
+      auto enc = SparseTensorEncodingAttr::get(
+          op->getContext(), encDst.getDimLevelType(), encDst.getDimOrdering(),
+          encSrc.getPointerBitWidth(), encSrc.getIndexBitWidth());
+      newParams(rewriter, params, op, enc, kToCOO, sizes, src);
       Value coo = genNewCall(rewriter, op, params);
+      params[3] = constantI32(
+          rewriter, loc, getOverheadTypeEncoding(encDst.getPointerBitWidth()));
+      params[4] = constantI32(
+          rewriter, loc, getOverheadTypeEncoding(encDst.getIndexBitWidth()));
       params[6] = constantI32(rewriter, loc, kFromCOO);
       params[7] = coo;
       rewriter.replaceOp(op, genNewCall(rewriter, op, params));
       return success();
     }
     if (!encDst && encSrc) {
       // This is sparse => dense conversion, which is handled as follows:
       //   dst = new Tensor(0);

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conversion_ptr.mlir

 // RUN: mlir-opt %s \
 // RUN:   --sparsification --sparse-tensor-conversion \
 // RUN:   --linalg-bufferize --convert-linalg-to-loops \
 // RUN:   --convert-vector-to-scf --convert-scf-to-std \
 // RUN:   --func-bufferize --tensor-constant-bufferize --tensor-bufferize \
 // RUN:   --std-bufferize --finalizing-bufferize --lower-affine \
 // RUN:   --convert-vector-to-llvm --convert-memref-to-llvm --convert-math-to-llvm \
 // RUN:   --convert-std-to-llvm --reconcile-unrealized-casts | \
 // RUN: mlir-cpu-runner \
 // RUN:  -e entry -entry-point-result=void  \
 // RUN:  -shared-libs=%mlir_integration_test_dir/libmlir_c_runner_utils%shlibext | \
 // RUN: FileCheck %s
 
 #DCSR  = #sparse_tensor.encoding<{
-  dimLevelType = [ "compressed", "compressed" ]
+  dimLevelType = [ "compressed", "compressed" ],
+  pointerBitWidth = 8,
+  indexBitWidth = 8
 }>
 
 #DCSC  = #sparse_tensor.encoding<{
   dimLevelType = [ "compressed", "compressed" ],
-  dimOrdering = affine_map<(i,j) -> (j,i)>
+  dimOrdering = affine_map<(i,j) -> (j,i)>,
+  pointerBitWidth = 64,
+  indexBitWidth = 64
 }>
 
 //
 // Integration test that tests conversions between sparse tensors,
-// where the dynamic sizes of the shape of the enveloping tensor
-// may change (the actual underlying sizes obviously never change).
+// where the pointer and index sizes in the overhead storage change
+// in addition to layout.
 //
 module {
 
   //
   // Helper method to print values array. The transfer actually
   // reads more than required to verify size of buffer as well.
   //
   func @dump(%arg0: memref<?xf64>) {
     %c = arith.constant 0 : index
     %d = arith.constant -1.0 : f64
     %0 = vector.transfer_read %arg0[%c], %d: memref<?xf64>, vector<8xf64>
     vector.print %0 : vector<8xf64>
     return
   }
 
   func @entry() {
     %t1 = arith.constant sparse<
       [ [0,0], [0,1], [0,63], [1,0], [1,1], [31,0], [31,63] ],
         [ 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0 ]> : tensor<32x64xf64>
     %t2 = tensor.cast %t1 : tensor<32x64xf64> to tensor<?x?xf64>
 
-    // Four dense to sparse conversions.
-    %1 = sparse_tensor.convert %t1 : tensor<32x64xf64> to tensor<?x?xf64, #DCSR>
-    %2 = sparse_tensor.convert %t1 : tensor<32x64xf64> to tensor<?x?xf64, #DCSC>
-    %3 = sparse_tensor.convert %t2 : tensor<?x?xf64> to tensor<?x?xf64, #DCSR>
-    %4 = sparse_tensor.convert %t2 : tensor<?x?xf64> to tensor<?x?xf64, #DCSC>
-
-    // Two cross conversions.
-    %5 = sparse_tensor.convert %3 : tensor<?x?xf64, #DCSR> to tensor<?x?xf64, #DCSC>
-    %6 = sparse_tensor.convert %4 : tensor<?x?xf64, #DCSC> to tensor<?x?xf64, #DCSR>
+    // Dense to sparse.
+    %1 = sparse_tensor.convert %t1 : tensor<32x64xf64> to tensor<32x64xf64, #DCSR>
+    %2 = sparse_tensor.convert %t1 : tensor<32x64xf64> to tensor<32x64xf64, #DCSC>
+
+    // Sparse to sparse.
+    %3 = sparse_tensor.convert %1 : tensor<32x64xf64, #DCSR> to tensor<32x64xf64, #DCSC>
+    %4 = sparse_tensor.convert %2 : tensor<32x64xf64, #DCSC> to tensor<32x64xf64, #DCSR>
 
     //
     // All proper row-/column-wise?
     //
     // CHECK: ( 1, 2, 3, 4, 5, 6, 7, -1 )
     // CHECK: ( 1, 4, 6, 2, 5, 3, 7, -1 )
-    // CHECK: ( 1, 2, 3, 4, 5, 6, 7, -1 )
-    // CHECK: ( 1, 4, 6, 2, 5, 3, 7, -1 )
     // CHECK: ( 1, 4, 6, 2, 5, 3, 7, -1 )
     // CHECK: ( 1, 2, 3, 4, 5, 6, 7, -1 )
     //
-    %m1 = sparse_tensor.values %1 : tensor<?x?xf64, #DCSR> to memref<?xf64>
-    %m2 = sparse_tensor.values %2 : tensor<?x?xf64, #DCSC> to memref<?xf64>
-    %m3 = sparse_tensor.values %3 : tensor<?x?xf64, #DCSR> to memref<?xf64>
-    %m4 = sparse_tensor.values %4 : tensor<?x?xf64, #DCSC> to memref<?xf64>
-    %m5 = sparse_tensor.values %5 : tensor<?x?xf64, #DCSC> to memref<?xf64>
-    %m6 = sparse_tensor.values %6 : tensor<?x?xf64, #DCSR> to memref<?xf64>
+    %m1 = sparse_tensor.values %1 : tensor<32x64xf64, #DCSR> to memref<?xf64>
+    %m2 = sparse_tensor.values %2 : tensor<32x64xf64, #DCSC> to memref<?xf64>
+    %m3 = sparse_tensor.values %3 : tensor<32x64xf64, #DCSC> to memref<?xf64>
+    %m4 = sparse_tensor.values %4 : tensor<32x64xf64, #DCSR> to memref<?xf64>
     call @dump(%m1) : (memref<?xf64>) -> ()
     call @dump(%m2) : (memref<?xf64>) -> ()
     call @dump(%m3) : (memref<?xf64>) -> ()
     call @dump(%m4) : (memref<?xf64>) -> ()
-    call @dump(%m5) : (memref<?xf64>) -> ()
-    call @dump(%m6) : (memref<?xf64>) -> ()
 
     // Release the resources.
-    sparse_tensor.release %1 : tensor<?x?xf64, #DCSR>
-    sparse_tensor.release %2 : tensor<?x?xf64, #DCSC>
-    sparse_tensor.release %3 : tensor<?x?xf64, #DCSR>
-    sparse_tensor.release %4 : tensor<?x?xf64, #DCSC>
-    sparse_tensor.release %5 : tensor<?x?xf64, #DCSC>
-    sparse_tensor.release %6 : tensor<?x?xf64, #DCSR>
+    sparse_tensor.release %1 : tensor<32x64xf64, #DCSR>
+    sparse_tensor.release %2 : tensor<32x64xf64, #DCSC>
+    sparse_tensor.release %3 : tensor<32x64xf64, #DCSC>
+    sparse_tensor.release %4 : tensor<32x64xf64, #DCSR>
 
     return
   }
 }