This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse][vectorization] optimize reduction chains
ClosedPublic

Authored by aartbik on Nov 22 2022, 10:50 AM.

Diff Detail

Event Timeline

aartbik created this revision.Nov 22 2022, 10:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 10:50 AM
aartbik requested review of this revision.Nov 22 2022, 10:50 AM
aartbik updated this revision to Diff 477254.Nov 22 2022, 10:54 AM

rebased main

qcolombet accepted this revision.Nov 22 2022, 3:51 PM

LGTM, nits below.

mlir/lib/Dialect/SparseTensor/Transforms/SparseVectorization.cpp
106

Thanks for adding that!

Do we have a verifier/assert for that?

146
192

Should we turn this into an assert?

This revision is now accepted and ready to land.Nov 22 2022, 3:51 PM
aartbik marked 3 inline comments as done.Nov 22 2022, 4:00 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseVectorization.cpp
106

I plan to add a few more checks in a follow up cl (that adds "index")

146

You are right!

This is one minor case where I disagree with style since for a chain of if-else with braces { } that form becomes so much longer.

But one cannot argue with rules, so done ;-)

192

no, assert is DEBUG only, unreachable is in PROD also

aartbik updated this revision to Diff 477338.Nov 22 2022, 4:12 PM
aartbik marked 3 inline comments as done.

addressed comments

qcolombet added inline comments.Nov 22 2022, 6:44 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseVectorization.cpp
146

This is one minor case where I disagree with style since for a chain of if-else with braces { } that form becomes so much longer.

+1 x)

192

If we want to keep that in PROD, we should switch to something like emitError or other because llvm_unreachable can also be stripped with some cmake options.

See https://github.com/llvm/llvm-project/blob/2dfe76e989877d3992bf52971f27ad4ae5064a6d/llvm/include/llvm/Support/ErrorHandling.h#L133

aartbik updated this revision to Diff 478066.Nov 26 2022, 12:32 PM

rebased with main

This revision was landed with ongoing or failed builds.Nov 26 2022, 12:41 PM
This revision was automatically updated to reflect the committed changes.