This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Factoring out Transforms/CodegenUtils.{cpp,h}
ClosedPublic

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

Details

Summary

This moves a bunch of helper functions from Transforms/SparseTensorConversion.cpp into Transforms/CodegenUtils.{cpp,h} so that they can be reused by Transforms/Sparsification.cpp, etc.

See also the dependent D115010 which cleans up some corner cases in this change.

Diff Detail

Event Timeline

wrengr created this revision.Dec 2 2021, 4:34 PM
wrengr requested review of this revision.Dec 2 2021, 4:35 PM
wrengr updated this revision to Diff 391510.Dec 2 2021, 5:49 PM

Updating the two workaround comments with the differential ID that removes them

wrengr edited the summary of this revision. (Show Details)Dec 3 2021, 4:15 PM
aartbik added inline comments.Dec 6 2021, 1:47 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
22–24 ↗(On Diff #391510)

This is a good explanation, but sort of a very well known technique.
I think a simple

// Forward references.
....

suffices ;-)

mlir/lib/Dialect/SparseTensor/Utils/CodegenUtils.cpp
16–18

// Forward.

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1810–1815

Is this autogen? Shouldn't we just add the right deps, instead of adding outside headers to this lib?

wrengr updated this revision to Diff 392186.Dec 6 2021, 2:31 PM
wrengr marked 2 inline comments as done.

addressing nits

wrengr added inline comments.Dec 6 2021, 2:34 PM
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1810–1815

I tried using build_cleaner (which made the changes to the deps below), but it couldn't figure out how to fulfill these headers without pulling in a huge plethora of other stuff. I'll try taking another whack at it

wrengr updated this revision to Diff 392203.Dec 6 2021, 3:16 PM

cleaning up BUILD.bazel

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1810–1815

Okay, I managed to get a decently clean version. The new version still has "include/mlir/ExecutionEngine/SparseTensorUtils.h" in the hdrs (which should be fine(?) since that's what :SparseTensorTransforms does as well). Alternatively we could add ":mlir_c_runner_utils" to the deps (which will also bring that header in, though at the cost of building all of :mlir_c_runner_utils).

aartbik added inline comments.Dec 6 2021, 8:16 PM
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1810–1815

Yeah, this look okay to me, since now we only include what we "own" in a sense.
Let's see if others chime in, but I am okay with this.

rriddle requested changes to this revision.Dec 6 2021, 8:18 PM
rriddle added inline comments.
mlir/lib/Dialect/SparseTensor/Utils/CodegenUtils.cpp
14–18

namespaces should only really have classes, please remove these and use static methods and full namespace resolution instead.

This revision now requires changes to proceed.Dec 6 2021, 8:18 PM
wrengr marked 2 inline comments as done.Dec 7 2021, 11:24 AM
wrengr updated this revision to Diff 392508.Dec 7 2021, 1:01 PM

adjusting namespaces

wrengr marked 2 inline comments as done.Dec 7 2021, 1:02 PM
rriddle added inline comments.Dec 7 2021, 1:42 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
1–11 ↗(On Diff #392508)

It isn't clear to me that we should be exposing this kind of API in the external API, can this just be local to the lib/ directory?

96–113 ↗(On Diff #392508)

I'm not really sure about this kind of API honestly, how much is it really saving? This comment isn't entirely blocking assuming this is local to lib/, but just a general comment.

100–103 ↗(On Diff #392508)

This looks unused.

22–24 ↗(On Diff #391510)

I wouldn't even add a comment, we don't do that anywhere else.

wrengr updated this revision to Diff 392879.Dec 8 2021, 12:36 PM
wrengr marked 2 inline comments as done.

addressing nit

mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
1–11 ↗(On Diff #392508)

I have no idea where the external API is specified. This code is intended to be internal to the sparse_tensor dialect, but for general reuse by that dialect (i.e., across both lib/Dialect/SparseTensor/Utils/*.cpp and lib/Dialect/SparseTensor/Transforms/*.cpp)

96–113 ↗(On Diff #392508)

It saves quite a lot actually. Saving 16–20 characters (46–54%) every single time we want to emit a constant adds up very quickly. Not to mention ensuring uniformity re which of the many different constant ops gets used. Let alone if we want to change that choice (as we did just recently). Not to mention the costs of onboarding new developers. It may have been some time since you saw MLIR with fresh eyes, but I assure you that for any newcomer to the codebase the constant drag from wading through the wall of text without these macros greatly impedes their velocity. And mere velocity is not the issue, but rather the fact that having such low initial velocity generally causes new developers to lose heart and decide the project isn't for them. If you want to discuss this further, we can set up a meeting to do so.

100–103 ↗(On Diff #392508)

At present, yes; though I have no reason to believe it will remain so, and it strikes me as unnecessarily baroque to punch a hole in the regularity of the API just to eliminate a one-liner.

rriddle requested changes to this revision.Dec 8 2021, 12:44 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
1–11 ↗(On Diff #392508)

Anything in include/mlir is considered an "external" API. This should be moved to the lib/ folder.

96–113 ↗(On Diff #392508)

It may have been some time since you saw MLIR with fresh eyes,

I'll start off by saying that I really don't appreciate the tone of this message

Saving 16–20 characters (46–54%) every single time we want to emit a constant adds up very quickly.

Some of these functions are unused, and some are used only a handful of times. If you want to save code, writing local utilities like this can be fine but I am somewhat strongly opposed to exposing stuff like in any kind of external API.

100–103 ↗(On Diff #392508)

I don't see a reason to add code that is untested, seems better to add it when it is actually necessary.

This revision now requires changes to proceed.Dec 8 2021, 12:44 PM
mehdi_amini added inline comments.Dec 8 2021, 12:56 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

It saves quite a lot actually. Saving 16–20 characters (46–54%) every single time we want to emit a constant adds up very quickly. [...]

Thanks for noticing this.

The usual process in MLIR is to raise this on Discourse, write an RFC, and gain consensus.
We should definitely look into this kind of areas of improvement, and each newcomer to the project has indeed a "fresh pair of eyes" that is unique and very valuable! (after a few months, most people get "used to things" even when they are suboptimal).

That said the things we've been consistent about is to keep all the common infrastructure shared and unified in the project: if you have a good case for helpers method to build various constant, it's unlikely that these have anything specific to the SparseTensor domain and so they really don't belong here.

aartbik added inline comments.Dec 8 2021, 1:34 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

It may have been some time since you saw MLIR with fresh eyes,

I'll start off by saying that I really don't appreciate the tone of this message

Wren, please don't say stuff like this. River is our top contributor with a great passion for keeping our source and organization clean and consistent.

River, I probably set the wrong example here by using include/mlir/Dialect/SparseTensor/Utils/ as the place to define the headers of utilities that are used in lib/mlir/Dialect/SparseTensor/* files, with the current (and only) example of Merger.h. I got that inspiration from several other dialects, e.g. include/mlir/Dialect/Linalg/Utils/Utils.h. I was not aware that defining headers in the lib directory is preferred in such places. Just for my understanding, do you think Merger.h was the right decision (since it it more elaborate and has its own testing) and is the objection purely based on having very tiny utils in the header, or do you prefer we move in that direction for everything?

wrengr updated this revision to Diff 392922.Dec 8 2021, 1:44 PM

rebase (over D115004 and D115005)

rriddle added inline comments.Dec 8 2021, 1:45 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

The distinction that has generally been drawn between include/ vs lib/ is what is considered an "implementation detail" of a library, and what is meant to be exposed as a general utility to external users of this library. If we just want to have a place to share code between different passes within the same library then lib/ is generally a better choice, given that the stipulations and expectations of internal API are much more relaxed than external API.

Merger could have also been defined in lib/ if it's not meant to be used by external clients, not sure what the expectation there is. Some of the GPU conversion utils are like this: https://github.com/llvm/llvm-project/tree/main/mlir/lib/Conversion/GPUCommon

I will note that you can still write targeted tests for things only defined in lib/, the test directory can use relative include for these types of things: https://github.com/llvm/llvm-project/blob/f638c4d6e4a28fbd5508f853a8715cc92bb66d48/mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp#L9

aartbik added inline comments.Dec 8 2021, 1:51 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

Merger could have also been defined in lib/ if it's not meant to be used by external clients, not sure what the expectation there is.

Thanks River for the background. That makes me feel better, since the expectation indeed was that other clients could start using this (since lattice operations are central to the TACO way of generating code), for example, when external contributor start writing improvements on sparse codegen, but want to use the same lattice theory.

Wren, I think we should do the following next
(1) take a look at our utils, see which one could be beneficial for all (a) and which one are specific to sparse
(2a) move (a) to a general place, with River's blessing
(2b) move (b) to a header inside SparseDialects lib

rriddle added inline comments.Dec 8 2021, 1:57 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

Right, exactly what Mehdi says here. The "external API" of MLIR (and to a lesser extent the "internal API") need to be consistent. As newcomers engage with the project there can be a tendency to claim "ahah, this is definitely the way it should be because it's easy for me". In many cases this can help the project grow, but it has to be something agreed upon by the community lest we develop pockets of consistency (as different parts of the project develop differently). For this specific API, we've had discussions in the past that never really reached maturity (https://llvm.discourse.group/t/evolving-builder-apis-based-on-lessons-learned-from-edsc/879), and maybe there can be some renewed interest in starting a discussion there. Until then though, we need to ensure the API of the project is consistent.

wrengr added inline comments.Dec 9 2021, 2:49 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

Aha! I'd thought the include/ vs lib/ split was just for *.h vs *.cpp file types. I'll move this over to lib/ then, since the intention wasn't to make it public API. Sorry for the confusion

96–113 ↗(On Diff #392508)

Yes of course, consensus is crucial for any major style/API changes. Again, my intent with this differential was never to change anything outside of the SparseTensor codebase, hence why I did not expect such pushback against this internal cleanup.

Both getOneAttr and genIsNonzero are of general utility, though the latter needs renaming (afaik the "gen" prefix is our own thing rather than mlir standard). I can write up RFCs for both of these, but I'd rather not have the current differential (or rather D115010, D115012) depend on those being resolved.

The constant generators are also of general utility, but they're far trickier to upstream (especially to do so without losing their brevity). There are several different versions of constant ops for different dialects, so we can't just make them into OpBuilder methods. I suppose we could make an ArithOpBuilder subclass, but since that's not an established pattern it strikes me as a terrible precedent. So there's considerable design work to be done just to determine what it would even mean to make these public, let alone reaching consensus. It would be great to do that work, but that's a considerably larger undertaking than what this differential aims to accomplish.

That said, I'm getting the impression that I'm forbidden to do any sort of code cleanup that could be construed as general purpose. I fully appreciate the importance of avoiding design islands, believe me. Yet every other open-source project I have worked on has had it as part of their RFC process to demonstrate the utility of a change, namely by implementing it and using it on a small scale. If such small-scale implementations are forbidden, then I have difficulty seeing how any progress can be made.

wrengr updated this revision to Diff 393308.Dec 9 2021, 3:44 PM

Moved CodegenUtils.h into lib/. Dunno if the "../Utils/CodegenUtils.h" is acceptable style, or if there's a better way. Also rebased.

wrengr updated this revision to Diff 393328.Dec 9 2021, 4:17 PM

added todos for filing RFCs to moving things upstream

mehdi_amini added inline comments.Dec 9 2021, 4:30 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/CodegenUtils.h
96–113 ↗(On Diff #392508)

The difficulty is how to manage these "islands" and avoid these to be creeping up. So in general I'm fairly conservative in trying to prevent them from existing in the first place (there are enough out-of-tree users to find such patterns through experiment) and instead be more proactive in refactoring our core infra.

We saw with EDSC that a small island quickly becomes its own continent and is really hard to reconcile with the rest of the codebase later, I think it took more than a year of cleanups to purge it (I don't even know if we succeeded entirely).

Another aspect is that letting islands emerge removes the incentive for folks to take the time to abstract away and generalize the concepts, so pushing back on a new "island" early is a way to keep a stronger incentive to creation motion around things like https://llvm.discourse.group/t/evolving-builder-apis-based-on-lessons-learned-from-edsc/879

In the end I'm not sure there is a perfect answer but it ends up being about various tradeoffs to make...

aartbik added inline comments.Dec 10 2021, 9:52 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
17

This looks strange. Also, because CodeGenUtils.* is now a private facing API that supports only our codegen in Ttransforms, it seems to me all the the files should simply live in lib/Dialect/SparseTensor/Transforms, i.e. local to our codegen, since Utils should be reserved for implementation of our more public facing support.

@rriddle @mehdi_amini can you please advise?

mehdi_amini added inline comments.Dec 10 2021, 12:28 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
17

Either way is fine with me.

aartbik added inline comments.Dec 10 2021, 12:31 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
17

If either is fine with the masters, then I have a slight preference moving all the CodeGenUtils.* into Transforms, Wren. For now, I think you can even keep the "general" constant getters there, and go through the RFC of making them more widely available if you feel like going that route. But having these, now private, convenience methods inside the sparse codegen for now seems totally acceptable for the time being.

wrengr updated this revision to Diff 394691.Dec 15 2021, 5:20 PM
wrengr marked 3 inline comments as done.

Moving CodegenUtils.{h,cpp} into Transforms/ instead of Utils/.
Also rebasing

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
17

The only issue I can think of re moving CodegenUtils into Transforms/ is that Utils/Merger.cpp also uses it. It's only a very minor issue since, iirc, Merger.cpp only uses one of the simpler functions once; so I'm totally cool with undoing that change for the sake of cleaning up the structure of things in Transforms/*. But it is something to bear in mind in the future depending on how Merger.* and CodegenUtils.* evolve.

wrengr retitled this revision from [mlir][sparse] Factoring out Utils/CodegenUtils.{cpp,h} to [mlir][sparse] Factoring out Transforms/CodegenUtils.{cpp,h}.Dec 15 2021, 5:22 PM
wrengr edited the summary of this revision. (Show Details)
aartbik added inline comments.Dec 15 2021, 9:13 PM
mlir/lib/Dialect/SparseTensor/Transforms/CMakeLists.txt
2 ↗(On Diff #394691)

I believe you have to list the header file here too now (others please chime in if that is wrong)

wrengr added inline comments.Dec 16 2021, 1:27 PM
mlir/lib/Dialect/SparseTensor/Transforms/CMakeLists.txt
2 ↗(On Diff #394691)

It does build without listing the header... And none of the other lib/Dialect/**/CMakeLists.txt list their internal headers (albeit, all their internal headers are named PassDetail.h except for two instances of TypeDetail.h, so I don't know if those are considered special)... So I don't think it should be listed, but then I know very little about cmake so I'd rather get input from someone who does

aartbik added inline comments.Dec 20 2021, 10:18 AM
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1821

Why does the sparse tensor utils depend on sparse tensors now and not the other way around. like it used to?
Is this still required with the new structure? It feels a bit the wrong way to me.

wrengr added inline comments.Dec 20 2021, 11:48 AM
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1821

Imo, it makes the most sense to have the definition of a language be a priori to and independent of any utilities/transformations on that language. The naturalness of this layering is evidenced by the fact that the IR library doesn't actually depend on the utils at all (cf., line 1807 above), as well as the utils needing to pull in the IncGen dependencies if it can't get them from the IR library (which strikes me as a failure/violation of cohesion/encapsulation).

But I can revert the change if you feel strongly

aartbik added inline comments.Dec 20 2021, 12:12 PM
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1821

I suppose that is true. I was thinking mainly of the new utils that support IR building, but there are also utils that indeed use the sparse encoding. Note that part of this is also because the sparse tensor build rule is too large, I really would like to see the sparse tensor attribute being independent of the dialect, especially since this may be used outside the sparse tensor dialect without having to pull in everything (see issue https://github.com/llvm/llvm-project/issues/52748 I filed recently).

aartbik accepted this revision.Dec 20 2021, 12:12 PM
wrengr added inline comments.Dec 20 2021, 12:29 PM
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
1821

Making the attributes independent of the rest of the dialect makes sense to me. Nicely separates the type definition from the operations, avoiding the analogue of the forward class declaration hack (and opening the way to split up the sparse_tensor dialect or define alternatives to it, if we needed either for some reason).

So, should I revert the BUILD.bazel change or no?

I am okay with BUILD file as is right now. I think we may bikeshed about this later when we extract encoding out ;-)

rriddle accepted this revision.Jan 4 2022, 1:41 PM

I don't have any concerns after moving to lib/, deferring the approval for this dir to Aart.

This revision is now accepted and ready to land.Jan 4 2022, 1:41 PM