This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Add layout to the memref for the indices buffers to prepare for the AOS storage optimization for COO regions.
ClosedPublic

Authored by bixia on Dec 28 2022, 1:36 PM.

Diff Detail

Event Timeline

bixia created this revision.Dec 28 2022, 1:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
bixia requested review of this revision.Dec 28 2022, 1:36 PM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
76 ↗(On Diff #485544)

Why does it have to be unique to apply AOS?

bixia updated this revision to Diff 485628.Dec 29 2022, 11:28 AM

Move getCooStart to SparseTensor.h.

bixia added inline comments.Dec 29 2022, 11:31 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
76 ↗(On Diff #485544)

This ensures the the last dim is a unique singleton.
What does it mean if the last dim is a non-unique singleton? If that is also a meaningful case we want to support, we can relax the requirement here.

aartbik added inline comments.Dec 29 2022, 1:14 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h
48

Empty line around each individual method (style in this file)

54

this feels a bit too long for linline, also move to cpp

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
276

we probably need to assert s vs. rank, just to make sure this does not return the wrong value when passing in the wrong value for s

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

now only 2 out of 3 getters use the SparseGetter abstraction? Is it still worth it. Shall we just move that back into the other rewriting rules too and make it more regular?

bixia updated this revision to Diff 485645.Dec 29 2022, 2:12 PM
bixia marked 4 inline comments as done.

Move getCooStart to cpp. Remove SparseGetter. Add an assertion to check the input dim.

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
276

getDimLevelType has such an assert, but we add an assert here also.

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

remove SparseGetter then.

Peiming added inline comments.Dec 29 2022, 2:16 PM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
141

Why not create a new builder for ToPointer/IndiceOp which infers the type?

It would be cleaner.

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
76 ↗(On Diff #485544)

It would become a BCOO

bixia updated this revision to Diff 485647.Dec 29 2022, 2:55 PM

Rename getCooStart to getCOOStart to be consistent with other routines. Change the routine to not require unique last dim.

bixia marked an inline comment as done.Dec 29 2022, 2:56 PM
bixia added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
141

I will do this in another CL, to add builders to ToPointersOp, ToValuesOp, and ToIndicesOp that infer the result types.

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h
76 ↗(On Diff #485544)

Remove the requirement for unique.

bixia updated this revision to Diff 485917.Jan 2 2023, 9:40 PM
bixia marked an inline comment as done.

Add codegen utils for inferring the result types and generate ToValues/Indices/PointersOp.

bixia added inline comments.Jan 3 2023, 11:02 AM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
141

I realized adding builder methods to infer result type will create a dependence from SparseTensorDialect.cpp to CodeGenUtils.cpp/h, which is what we want to avoid. Add a codegenutil instead.

bixia updated this revision to Diff 486081.Jan 3 2023, 2:06 PM

Replace "tailing" with "trailing".

aartbik added inline comments.Jan 3 2023, 2:43 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
279

This comment is now outdated since you have the isUnique override parameter. You should really say something like, if unique is requested then ....

288

by scanning only up to rank-1, you ensure there is always a compressed + one or more singleton

this is getting subtle, so really needs proper doc (not just in API, but here in implementation)

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

I would not say that you add a layout when set but

When withLayout is set, the returned layout has unknown strides and offsets. Otherwise,
the standard unit stride zero offset layout is returned.

or something like that

mlir/test/Dialect/SparseTensor/sorted_coo.mlir
74–75

why not just complete the type? it does not vary right?

Leaving an < unclosed feels a bit wrong

bixia updated this revision to Diff 486106.Jan 3 2023, 3:17 PM
bixia marked 4 inline comments as done.

Fix/add documentation. Complete the strided memref in FileCheck tests.

bixia added inline comments.Jan 3 2023, 3:18 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
288

The document for the routine also mentions this, add doc here:

We only consider COO region with at least two dimensions for the purpose
of AOS storage optimization.

mlir/test/Dialect/SparseTensor/sorted_coo.mlir
74–75

Fix here an other places.

aartbik accepted this revision.Jan 3 2023, 3:33 PM
This revision is now accepted and ready to land.Jan 3 2023, 3:33 PM