This is an archive of the discontinued LLVM Phabricator instance.

Differential D138226

[mlir][sparse] Fix a problem in the new operator rewriter.
ClosedPublic

Authored by bixia on Nov 17 2022, 10:20 AM.

Details

Reviewers
aartbik
nicolasvasilache
Peiming
Commits
rG96b3bf429273: [mlir][sparse] Fix a problem in the new operator rewriter.
Summary

The getSparseTensorReaderNextX functions should return void.

Diff Detail

Event Timeline

bixia created this revision.Nov 17 2022, 10:20 AM
Herald added a reviewer: aartbik. · View Herald TranscriptNov 17 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: Moerafaat, anlunx, bzcheeseman and 21 others. · View Herald Transcript
bixia requested review of this revision.Nov 17 2022, 10:20 AM
Herald added a reviewer: nicolasvasilache. · View Herald TranscriptNov 17 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
bixia added a reviewer: Peiming.Nov 17 2022, 10:26 AM
Harbormaster completed remote builds in B198245: Diff 476171.Nov 17 2022, 10:39 AM
Peiming added inline comments.Nov 17 2022, 11:42 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
933–934

If resultType is empty, why is it safe to call getResult(0) here?

933–934

Plus, you did not catch the value anyway.

aartbik accepted this revision.Nov 17 2022, 12:30 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
933–934

+1, just remove the getResult call

This revision is now accepted and ready to land.Nov 17 2022, 12:30 PM
bixia updated this revision to Diff 476211.Nov 17 2022, 12:42 PM

Remove unnecessary getResult(0).

Harbormaster completed remote builds in B198275: Diff 476211.Nov 17 2022, 12:57 PM
Closed by commit rG96b3bf429273: [mlir][sparse] Fix a problem in the new operator rewriter. (authored by bixia). · Explain WhyNov 17 2022, 1:21 PM
This revision was automatically updated to reflect the committed changes.
bixia added a commit: rG96b3bf429273: [mlir][sparse] Fix a problem in the new operator rewriter..

Revision Contents

PathSize
mlir/
lib/
Dialect/
SparseTensor/
Transforms/
5 lines
test/
Integration/
Dialect/
SparseTensor/
CPU/
19 lines

Diff 476217

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

Show First 20 Lines • Show All 924 Lines • ▼ Show 20 Lines LogicalResult matchAndRewrite(NewOp op,
Value value = genAllocaScalar(rewriter, loc, eltTp); Value value = genAllocaScalar(rewriter, loc, eltTp);
scf::ForOp forOp = rewriter.create<scf::ForOp>(loc, c0, nnz, c1, scf::ForOp forOp = rewriter.create<scf::ForOp>(loc, c0, nnz, c1,
ArrayRef<Value>(cooBuffer)); ArrayRef<Value>(cooBuffer));
rewriter.setInsertionPointToStart(forOp.getBody()); rewriter.setInsertionPointToStart(forOp.getBody());
SmallString<18> getNextFuncName{"getSparseTensorReaderNext", SmallString<18> getNextFuncName{"getSparseTensorReaderNext",
primaryTypeFunctionSuffix(eltTp)}; primaryTypeFunctionSuffix(eltTp)};
Value indices = dimSizes; // Reuse the indices memref to store indices. Value indices = dimSizes; // Reuse the indices memref to store indices.
createFuncCall(rewriter, loc, getNextFuncName, {eltTp}, createFuncCall(rewriter, loc, getNextFuncName, {}, {reader, indices, value},
{reader, indices, value}, EmitCInterface::On) EmitCInterface::On);
PeimingUnsubmitted
Not Done
ReplyInline Actions

If resultType is empty, why is it safe to call getResult(0) here?

Peiming: If resultType is empty, why is it safe to call `getResult(0)` here?
PeimingUnsubmitted
Not Done
ReplyInline Actions

Plus, you did not catch the value anyway.

Peiming: Plus, you did not catch the value anyway.
aartbikUnsubmitted
Not Done
ReplyInline Actions

+1, just remove the getResult call

aartbik: +1, just remove the getResult call
.getResult(0);
SmallVector<Value> indicesArray; SmallVector<Value> indicesArray;
for (uint64_t i = 0; i < rank; i++) { for (uint64_t i = 0; i < rank; i++) {
indicesArray.push_back(rewriter.create<memref::LoadOp>( indicesArray.push_back(rewriter.create<memref::LoadOp>(
loc, indices, constantIndex(rewriter, loc, i))); loc, indices, constantIndex(rewriter, loc, i)));
} }
Value v = rewriter.create<memref::LoadOp>(loc, value); Value v = rewriter.create<memref::LoadOp>(loc, value);
Value t = rewriter.create<InsertOp>(loc, v, forOp.getRegionIterArg(0), Value t = rewriter.create<InsertOp>(loc, v, forOp.getRegionIterArg(0),
indicesArray); indicesArray);
▲ Show 20 Lines • Show All 129 Lines • Show Last 20 Lines

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

// RUN: mlir-opt %s --sparse-compiler | \ // DEFINE: %{option} = enable-runtime-library=true
// RUN: TENSOR0="%mlir_src_dir/test/Integration/data/test_symmetric_complex.mtx" \ // DEFINE: %{command} = mlir-opt %s --sparse-compiler=%{option} | \
// RUN: mlir-cpu-runner \ // DEFINE: TENSOR0="%mlir_src_dir/test/Integration/data/test_symmetric_complex.mtx" \
// RUN: -e entry -entry-point-result=void \ // DEFINE: mlir-cpu-runner \
// RUN: -shared-libs=%mlir_lib_dir/libmlir_c_runner_utils%shlibext | \ // DEFINE: -e entry -entry-point-result=void \
// RUN: FileCheck %s // DEFINE: -shared-libs=%mlir_lib_dir/libmlir_c_runner_utils%shlibext | \
// DEFINE: FileCheck %s
//
// RUN: %{command}
//
// Do the same run, but now with direct IR generation.
// REDEFINE: %{option} = enable-runtime-library=false
// RUN: %{command}
!Filename = !llvm.ptr<i8> !Filename = !llvm.ptr<i8>
#SparseMatrix = #sparse_tensor.encoding<{ #SparseMatrix = #sparse_tensor.encoding<{
dimLevelType = [ "compressed", "compressed" ] dimLevelType = [ "compressed", "compressed" ]
}> }>
#trait_sum_reduce = { #trait_sum_reduce = {
▲ Show 20 Lines • Show All 70 Lines • Show Last 20 Lines