This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Move helper function to work with IteratorTypes to StructuredOpsUtils.
AbandonedPublic

Authored by olegshyshkov on Nov 4 2022, 6:46 AM.

Details

Summary

isParallelIterator and isReductionIterator are moved from Linalg/Utils to
the new location so they can be used in LinalgOps itself.

getNParallelIterators is a very common pattern that was recreated 3 times in
the MLIR codebase.

Diff Detail

Event Timeline

olegshyshkov created this revision.Nov 4 2022, 6:46 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
olegshyshkov requested review of this revision.Nov 4 2022, 6:46 AM
mravishankar requested changes to this revision.Nov 4 2022, 12:52 PM
mravishankar added inline comments.
mlir/include/mlir/Dialect/Utils/StructuredOpsUtils.h
125

I find it strange that there is a utils namespace for these, but other methods here are not in utils namespace. Either we should move all of these to utils namespace, or drop the namespace altogether.

139

I am not sure this is anything more than return SmallVector<StringRef>(nParallel, getParallelIteratorType(). Dont think that needs a utility function.

This revision now requires changes to proceed.Nov 4 2022, 12:52 PM
olegshyshkov marked an inline comment as done.Nov 4 2022, 2:51 PM
olegshyshkov added inline comments.
mlir/include/mlir/Dialect/Utils/StructuredOpsUtils.h
125

I totally agree that it's not great and everything in this file should be under under. I checked what other methods are in this file and looks like there is more clean up to be done.

  • Getters for attribute names should probably go away, because they duplicate what is already generated by tablegen.
  • Getters for IteratorType names will got away once I'm done with this RFC.

So there is no reason to go over the same places in code twice. Would you unblock this patch if I leave a TODO and clean up in the following changes?

139

It is indeed just a SmallVector construction, but in my opinion it greatly improves readability and hides unnecessary verbose details when ops are constructed. This function was copied multiple time to different parts of the codebase and is heavily used, so I would think it's useful.

Would you suggest to inline all the existing usages instead?

Getters for attribute names should probably go away, because they duplicate what is already generated by tablegen.

+1!

Getters for IteratorType names will got away once I'm done with this RFC.

+1!

Would you suggest to inline all the existing usages instead?

That would be great. Lesser API surface area, the better.

Would you unblock this patch if I leave a TODO and clean up in the following changes?

I am only looking for consistency. In other dialects that provide utils (like Arith dialect and Tensor dialect) I dont see the util namespace being used (is util used anywhere in the codebase?). It just makes it confusing for developers looking around w.r.t. when to add util and when not to do so.
I'd suggest you just drop the util and save yourself some headache (unless you really care about it and want to establish some guidelines across MLIR about using of util namespace, which would also be fine by me)

olegshyshkov marked an inline comment as done.Nov 9 2022, 2:50 AM

I expected that this patch will help reduce the size of https://reviews.llvm.org/D137658, but the gain is so small, I don't think it's worth proceeding. I'll just drop this change.

olegshyshkov abandoned this revision.Nov 9 2022, 2:51 AM