This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Making `TensorExp::Kind` a nested enum-class
ClosedPublic

Authored by wrengr on Mar 14 2023, 1:15 PM.

Details

Summary

This improves namespacing, and follows the pattern used for "Kind" enums elsewhere in MLIR.

Diff Detail

Event Timeline

wrengr created this revision.Mar 14 2023, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:15 PM
wrengr requested review of this revision.Mar 14 2023, 1:15 PM
wrengr updated this revision to Diff 505239.Mar 14 2023, 1:17 PM

git-clang-format

aartbik added inline comments.Mar 20 2023, 2:23 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
91

how do you feel about having kind defined inside the class
(I think I know the answer given how you did it, but just checking)

aartbik accepted this revision.Mar 20 2023, 2:26 PM
This revision is now accepted and ready to land.Mar 20 2023, 2:26 PM
wrengr added inline comments.Mar 20 2023, 3:19 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
91

I think it reads better with the definition floated out, because the enum has so very many entries and thus inlining it would distract too much from the rest of the (very short) definition of TensorExp. (And I imagine those legibility concerns will only get worse whenever I get around to encoding the arity into the enum values.)

aartbik added inline comments.Mar 20 2023, 3:52 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
91

Makes sense. LGTM