This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Code cleanup for SparseTensorConversion
ClosedPublic

Authored by wrengr on Dec 2 2021, 4:15 PM.

Diff Detail

Event Timeline

wrengr created this revision.Dec 2 2021, 4:15 PM
wrengr requested review of this revision.Dec 2 2021, 4:15 PM
aartbik added inline comments.Dec 3 2021, 9:37 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
145

Add a /// comment

174

same

181

same

216

/*emitCInterface=*/
was dropped

320–321

what does this mean?

392

/*emitCInterface=*/t

798

/*emitCInterface=*/t

826

/*emitCInterface=*/t

856

/*emitCInterface=*/t

905

/*emitCInterface=*/t

wrengr updated this revision to Diff 391748.Dec 3 2021, 2:58 PM
wrengr marked 10 inline comments as done.

Addressing comments

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
320–321

I mean that if the elemTp is not one we support then constantPrimaryTypeEncoding will call llvm_unreachable. This isn't a change from previous behavior, just a comment about it. Really it's just a note-to-self I forgot to remove re the same concern discussed at D115012 re calling llvm_unreachable vs returning failure().

wrengr updated this revision to Diff 391767.Dec 3 2021, 3:58 PM

one more cleanup (inlined single-use Location variable)

aartbik accepted this revision.Dec 6 2021, 12:50 PM

Looking good!

This revision is now accepted and ready to land.Dec 6 2021, 12:50 PM
This revision was automatically updated to reflect the committed changes.