This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Factoring out NewCallParams class in SparseTensorConversion.cpp
ClosedPublic

Authored by wrengr on Nov 8 2022, 4:48 PM.

Details

Summary

The new class helps encapsulate the arguments to _mlir_ciface_newSparseTensor so that client code doesn't depend on the details of the API. (This makes way for the next differential which significantly alters the API.)

Diff Detail

Event Timeline

wrengr created this revision.Nov 8 2022, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 4:48 PM
wrengr requested review of this revision.Nov 8 2022, 4:48 PM
aartbik accepted this revision.Nov 8 2022, 5:09 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
250

I would be okay keeping this method inline as well. I can see why you moved it out, so that the class is perhaps a bit more readable, but having it all in one place makes sense to me too. But your call.

This revision is now accepted and ready to land.Nov 8 2022, 5:09 PM
wrengr added inline comments.Nov 8 2022, 5:20 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
250

I mainly left the definition outside the class in order to minimize the diff. Though since it is a rather substantial definition, I'm not sure moving it into the class would really help legibility (especially since all the other methods are extremely short). I think I'll leave it for now, and if anyone feels strongly they can always move it into the class later :)