This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Refactoring: abstract sparse tensor memory scheme into a SparseTensorDescriptor class.
ClosedPublic

Authored by Peiming on Nov 23 2022, 4:50 PM.

Details

Summary

This patch abstracts sparse tensor memory scheme into a SparseTensorDescriptor class. Previously, the field accesses are performed in a relatively error-prone way, this patch hides the hairy details behind a SparseTensorDescriptor class to allow users access sparse tensor fields in a more cohesive way.

Diff Detail

Event Timeline

Peiming created this revision.Nov 23 2022, 4:50 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Nov 23 2022, 4:50 PM
Peiming updated this revision to Diff 477656.Nov 23 2022, 4:56 PM

minor improvement.

Peiming updated this revision to Diff 477663.Nov 23 2022, 5:56 PM

minor fixes.

Peiming updated this revision to Diff 479047.Nov 30 2022, 12:21 PM

code cleanup.

Peiming updated this revision to Diff 479113.Nov 30 2022, 5:14 PM

replaces more places using SparseTensorDescriptor.

Peiming updated this revision to Diff 479170.Nov 30 2022, 10:15 PM

add some comments.

Peiming updated this revision to Diff 479330.Dec 1 2022, 9:21 AM

replace all SmallVectorImpl to SparseTensorDescriptor

Peiming edited the summary of this revision. (Show Details)Dec 1 2022, 9:21 AM
Peiming updated this revision to Diff 479340.Dec 1 2022, 9:55 AM
Peiming edited the summary of this revision. (Show Details)

minor fixes.

aartbik added inline comments.Dec 1 2022, 9:57 AM
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
99

This description describes the layout and thus drives all the code.
Can you please move this into the header, inside the class documentation.

Peiming updated this revision to Diff 479347.Dec 1 2022, 10:17 AM

add some comments.

Peiming updated this revision to Diff 479353.Dec 1 2022, 10:33 AM
Peiming marked an inline comment as done.

cleanup.

Peiming added inline comments.Dec 1 2022, 11:31 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
167

A better way to create PushBackOp can be PushBackOp(builder, loc, descriptor, fidx, value), which is more clear and can be read as "push a value into the sparse tensor descriptor at the given field index). But it would require us to put SparseTensorDescriptor to a publicly available place (and I am not sure whether it is wanted).

Peiming updated this revision to Diff 479432.Dec 1 2022, 2:27 PM

cleanup.

Peiming updated this revision to Diff 479434.Dec 1 2022, 2:36 PM

code cleanup.

aartbik added inline comments.Dec 2 2022, 4:29 PM
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h
336

I think this line should not be here? Copied from somewhere else?

357

relying *on* ad-hoc

377

, and a
(Oxford comma ;-)

387

an array

390

layouted -> laid out? (I think that is the proper english, not sure though...)

412

to restore (no d)

437

it feels like we have many more getters than we really need?
am I right, is there a way to condense the API a bit?

441

Although this solution is much better at information hiding than the original, we now need to scan each time for every field query. Although this is never very deep (rank bounded), it is still a bit wasteful. Can we avoid this by precomputing offsets per dimenison?

448

this feels very convoluted? why do we need a dim for a value query?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
251

note that this works for now, but we actually planned to implement a heuristic here, which will need to scan the ranks + level types again

273

continue

386

yeah, agreed, this made more sense in the original

but the alternative is to copy and create a fully new array....

Peiming updated this revision to Diff 479780.Dec 2 2022, 4:45 PM
Peiming marked 9 inline comments as done.

remove unused APIs + fix typos.

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h
441

I also think about it, we could, but storing the map will make the class expensive to copy.

How about I do it in next revision and change all the descriptor to reference?

448

deleted.

Peiming added inline comments.Dec 2 2022, 5:08 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
251

Then, you can have another foreach before this to compute the heuristic.

aartbik accepted this revision.Dec 5 2022, 2:08 PM
This revision is now accepted and ready to land.Dec 5 2022, 2:08 PM
This revision was landed with ongoing or failed builds.Dec 5 2022, 2:12 PM
This revision was automatically updated to reflect the committed changes.

@Peiming, this patch breaks the flang build.

@Peiming, this patch breaks the flang build.

It is not flang specific issue, though. The following gcc buildbot fails: https://lab.llvm.org/buildbot/#/builders/160/builds/13724

@Peiming, this patch breaks the flang build.

It is not flang specific issue, though. The following gcc buildbot fails: https://lab.llvm.org/buildbot/#/builders/160/builds/13724

This also broke the windows mlir buildbot. Please address the issues or revert.

@Peiming, this patch breaks the flang build.

It is not flang specific issue, though. The following gcc buildbot fails: https://lab.llvm.org/buildbot/#/builders/160/builds/13724

This also broke the windows mlir buildbot. Please address the issues or revert.

Okay, Investigating

The failure seems unrelated to this patch...

@Peiming, this patch breaks the flang build.

It is not flang specific issue, though. The following gcc buildbot fails: https://lab.llvm.org/buildbot/#/builders/160/builds/13724

This also broke the windows mlir buildbot. Please address the issues or revert.

The windows failure should be fixed by https://reviews.llvm.org/D139383

The failure seems unrelated to this patch...

The build log seems to directly point to your code (https://lab.llvm.org/buildbot/#/builders/160/builds/13724/steps/5/logs/stdio):

In file included from ../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/SparseBufferRewriting.cpp:14:
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:396:13: error: explicit specialization in non-namespace scope ‘class mlir::sparse_tensor::SparseTensorDescriptorImpl<mut>’
  396 |   template <>
      |             ^
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:397:10: error: too few template-parameter-lists
  397 |   struct ArrayStorage<false> {
      |          ^~~~~~~~~~~~~~~~~~~
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:401:13: error: explicit specialization in non-namespace scope ‘class mlir::sparse_tensor::SparseTensorDescriptorImpl<mut>’
  401 |   template <>
      |             ^
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:402:10: error: too few template-parameter-lists
  402 |   struct ArrayStorage<true> {
      |          ^~~~~~~~~~~~~~~~~~

The failure seems unrelated to this patch...

The build log seems to directly point to your code (https://lab.llvm.org/buildbot/#/builders/160/builds/13724/steps/5/logs/stdio):

In file included from ../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/SparseBufferRewriting.cpp:14:
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:396:13: error: explicit specialization in non-namespace scope ‘class mlir::sparse_tensor::SparseTensorDescriptorImpl<mut>’
  396 |   template <>
      |             ^
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:397:10: error: too few template-parameter-lists
  397 |   struct ArrayStorage<false> {
      |          ^~~~~~~~~~~~~~~~~~~
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:401:13: error: explicit specialization in non-namespace scope ‘class mlir::sparse_tensor::SparseTensorDescriptorImpl<mut>’
  401 |   template <>
      |             ^
../llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h:402:10: error: too few template-parameter-lists
  402 |   struct ArrayStorage<true> {
      |          ^~~~~~~~~~~~~~~~~~

Okay, I did not see this. I will try with GCC

@Peiming : I reverted your change because it broke multiple buildbots - windows and gcc included. I see that you've re-committed it. Did you make sure to address all three build breaks that were reported including the one @vzakhari pointed out? In cases like this, please make sure NOT to recommit your changes without making sure that all build breaks are addressed as it is disruptive to have buildbots that are broken especially when a change is identified as the source.

Peiming added a comment.EditedDec 5 2022, 5:40 PM

@Peiming : I reverted your change because it broke multiple buildbots - windows and gcc included. I see that you've re-committed it. Did you make sure to address all three build breaks that were reported including the one @vzakhari pointed out? In cases like this, please make sure NOT to recommit your changes without making sure that all build breaks are addressed as it is disruptive to have buildbots that are broken especially when a change is identified as the source.

Yes, of course. I haven't push the code, waiting the pre-merge windows build to finish.