This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Improving SparseTensorDimSliceAttr methods
ClosedPublic

Authored by wrengr on May 18 2023, 5:16 PM.

Details

Summary

This patch makes the following changes to SparseTensorDimSliceAttr methods:

  • Mark isDynamic constexpr.
  • Add new helpers getStatic and getStaticString to avoid repetition.
  • Moved the definitions for getStatic{Offset,Stride,Size} and isCompletelyDynamic out of the class declaration; because there's no benefit to inlining them.
  • Changed parse to use kDynamic rather than literals.
  • Changed verify to use the isDynamic helper.

Diff Detail

Event Timeline

wrengr created this revision.May 18 2023, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 5:16 PM
wrengr requested review of this revision.May 18 2023, 5:16 PM
Peiming added inline comments.May 18 2023, 5:24 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
89

Why call getStaticString when the v can be dynamic?

wrengr added inline comments.May 18 2023, 8:07 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
89

Perhaps it's not the best name, but here's the reasoning that lead to it: When factoring out the code duplication part of getStatic{Offset,Stride,Size}, it makes sense to call the new method "getStatic". In English, the duplicate code in the print method was essentially saying "if getV() is static, then to_string it, else return '?'". In pseudocode we can rephrase that to "getString(getStatic(getV()))". But then rather than defining "getString" it'd make more sense to define the composite getString*getStatic. And that composite is just like getStatic except for returning a string, hence getStaticString.

The big naming problem here is that we don't have a name/class that comprises all three of {Offset,Stride,Size}. If we did, then we could name it "DSA::fooToString(Foo)", "DSA::getFooString(Foo)", "Foo::getString()", or similar.

I'm open to suggestions if you have a better name in mind

Peiming added inline comments.May 19 2023, 8:44 AM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
89

I also don’t think getStaticString should be a member function either. (I can not think of other place where this function is needed outside the op::print). It might better be a file local utils.

BTW, I also find that ShapedType has both kDynamicSize and KDynamicOffsetOrStride, is it better to reuse those here?

wrengr added inline comments.May 19 2023, 12:07 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
89

Yeah, ideally I feel like all four of {kDynamic,isDynamic,getStatic,getStaticString} should really just be file-local rather than being part of the class, since none of them are needed outside of the {parse,verify,print} methods. The only reason I didn't do things that way was because I couldn't think of decent names for them, so keeping them under the SparseTensorDimSliceAttr namespace avoids the naming problem.

Again, I'm open to other suggestions. As the description says, the main goal of the CL is just to: (1) move defns out of the class decl; (2) reduce repetition; (3) have {parse,verify} use the helpers, not just for reducing repetition but also for ensuring that they remain in sync (e.g., if we decided to change kDynamic to being INT_MIN, or decided to change these parameters into newtypes or attributes, etc). So, I'm not wedded to the names or where the helpers live, just so long as they satisfy those goals.

89

BTW, I also find that ShapedType has both kDynamicSize and KDynamicOffsetOrStride, is it better to reuse those here?

As for whether to reuse ShapedType::kDynamic{Size,OffsetOrStride}: I think that's a good idea, since those're the abstractions used elsewhere in MLIR for the same task, therefore using them ourselves would help reduce conceptual complexity (and maybe even code duplication).

(Fwiw, I do have a CL in the wings for defining a new Size type that captures a bunch of the special logic associated with sizes. Whenever I get back to finishing it, it's the sort of thing I think should be upstreamed and reused by other dialects. I originally started working on it in order to simplify the code for the dim<->lvl stuff; but since it's a fair deal of code, I eventually figured out a way to remove it from the rest of the dim<->lvl stuff without introducing too much code duplication. I'm not suggesting the SparseTensorDimSliceAttr changes should wait until the Size code is in; just giving a random heads-up is all.)

aartbik accepted this revision.May 25 2023, 5:33 PM
This revision is now accepted and ready to land.May 25 2023, 5:33 PM
This revision was automatically updated to reflect the committed changes.