This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] code refactoring, move <tid, loop id> -> dim map to Merger.
ClosedPublic

Authored by Peiming on Oct 26 2022, 12:07 PM.

Diff Detail

Event Timeline

Peiming created this revision.Oct 26 2022, 12:07 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Oct 26 2022, 12:07 PM
Peiming edited the summary of this revision. (Show Details)Oct 26 2022, 12:08 PM
Peiming added reviewers: bixia, wrengr, anlunx.
aartbik added inline comments.Oct 27 2022, 10:41 AM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
250

I just noticed that the i and t appear in reverse order here, would you mind fixing that so all orders are consistent

260–261

same request on ordering here

261

Would you mind moving the geDimNum methods *after* the setDimLevelType, so that getDimLevelType and getDimLevelType remain grouped together

Peiming updated this revision to Diff 471257.Oct 27 2022, 12:13 PM
Peiming marked an inline comment as done.

rebase

mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
261

but setDimLevelType also set DimNum

I changed setDimLevelType to setDimAndDimLevelType, does it look better to you?

Peiming updated this revision to Diff 471258.Oct 27 2022, 12:14 PM

minor fix

aartbik accepted this revision.Oct 27 2022, 1:14 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
261

Ah, I see your logic. Related getters before related setters. Okay, SGTM.

281

Doc string?

This revision is now accepted and ready to land.Oct 27 2022, 1:14 PM
Peiming updated this revision to Diff 471288.Oct 27 2022, 1:53 PM

add comments.

This revision was landed with ongoing or failed builds.Oct 27 2022, 2:01 PM
This revision was automatically updated to reflect the committed changes.