This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] fix crash when using sparse_tensor::UnaryOp and ReduceOp.
ClosedPublic

Authored by Peiming on Jun 2 2023, 5:49 PM.

Diff Detail

Event Timeline

Peiming created this revision.Jun 2 2023, 5:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Jun 2 2023, 5:49 PM
Peiming updated this revision to Diff 528050.Jun 2 2023, 5:51 PM

revert unintended changes.

Peiming updated this revision to Diff 528051.Jun 2 2023, 5:52 PM

revert unintended changes.

aartbik added inline comments.Jun 2 2023, 5:58 PM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h
202–203

just an idea for later, but do we want to increase # tensors by one also?

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1496

typo: synthetic

1593

yeah, this is good logic

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_reductions_prod.mlir
144

repeating Wren's feedback on the typo originating from the other revision: "particular"

Peiming updated this revision to Diff 528055.Jun 2 2023, 6:01 PM
Peiming marked 4 inline comments as done.

address comments.

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h
202–203

Yeah! We should! Also it now matches the number of tensor in Merger. But I will do it in a separate patch...

wrengr added inline comments.Jun 2 2023, 6:02 PM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h
202–203

I think it'd be good to have the getNumTensors method give the +1 result; so that it stays consistent with the Merger method of the same name, as well as avoiding needing to spell out all these off-by-one cases

420

Moving this assertion defeats the purpose. We need to check the validity of tid before evaluating collapseReassoc[tid]

Peiming updated this revision to Diff 528056.Jun 2 2023, 6:07 PM
Peiming marked 2 inline comments as done.

address comments.

Peiming updated this revision to Diff 528057.Jun 2 2023, 6:08 PM

address comments.

aartbik accepted this revision.Jun 2 2023, 6:09 PM
This revision is now accepted and ready to land.Jun 2 2023, 6:09 PM
wrengr added inline comments.Jun 2 2023, 6:10 PM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
235–238

Nit: I think it'd be better to avoid saying "real" here, since that evokes the mathematical meaning of "real" meaning that the elements are real-numbers.

Often the antonym of "synthetic" is "analytic", though I don't think that's appropriate either (since it evokes other logical/mathematical things). Hmm... I would suggest "numOperands", but this includes the output tensor too... Perhaps, "numManifestTensors"?

Peiming updated this revision to Diff 528059.Jun 2 2023, 6:14 PM

renaming varaible.

Peiming updated this revision to Diff 528060.Jun 2 2023, 6:15 PM

fix typo.

This revision was landed with ongoing or failed builds.Jun 2 2023, 6:19 PM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B236336: Diff 528059.