Page MenuHomePhabricator

[mlir][Linalg] Fuse sequence of Linalg operation (on buffers)
ClosedPublic

Authored by mravishankar on Nov 6 2020, 4:33 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bondhugula added inline comments.Nov 9 2020, 9:40 PM
mlir/include/mlir/Dialect/Linalg/Passes.h
25 ↗(On Diff #303977)

The td description doesn't say as to what happens when this parameter is missing - there is no doc comment here either.

mlir/include/mlir/Dialect/Linalg/Passes.td
40 ↗(On Diff #303977)

Set the tile sizes to use -> Tiles sizes to use

Please don't use an imperative form here - instead just document what the parameter is.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
194–198

Why has the doc comment been deleted here?

This revision now requires changes to proceed.Nov 9 2020, 9:40 PM
mravishankar marked an inline comment as done.

Move pass to test pass.

mlir/include/mlir/Dialect/Linalg/Passes.h
25 ↗(On Diff #303977)

I moved the pass itself to test directory. This pass was added for testing.

mlir/include/mlir/Dialect/Linalg/Passes.td
40 ↗(On Diff #303977)

Moved to test.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
194–198

The original function was split to reuse functionality. The actual doc comment was moved down to the split function below. Adding comments here.

hanchung requested changes to this revision.Nov 12 2020, 12:52 AM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
204–206

nit drop braces for simple single-statement body of for

233–238

drop trivial braces

261

[optional]: I would name it pos because the body region is simple, and it's easy to know this is for producer because the right above line just states this.

548
551

ditto

579–581

nit: I would just return it directly. (the function name already states that it's returning consumerLoopToProducerLoop, so no need to capture it with a name for readability?)

588

The logic here only checks for contiguous pair. Is it possible to happen on non-contiguous pair? For example,

AffineMap: (d0, d1, d2) -> (d2, d1, d0)

and the fusableLoops only contains d0 and d2.

If it's must be contiguous, I'm confused that why this is a set not an integer or a pair-like type like [start, end).

588–593

This is my personal preference (and it's clearer that we are comparing with neighbors). How about rewrite like this:

for (unsigned curr = 1, e = map.getNumResults(); curr < e; ++curr) {
  unsigned posCurr = map.getResult(curr).cast<AffineDimExpr>().getPosition();
  unsigned posPrev = map.getResult(curr - 1).cast<AffineDimExpr>().getPosition();
  if (fusableLoops.count(posPrev) && fusableLoops.count(posCurr) && posPrev >= posCurr)
    return true;

With this way, we don't need to consider the relationship between i and j. It's more straightforward to me.

686

The TODO statement should be an actionable item. If this is a note, we don't need todo. If this is TODO, then what to do?

842

add assert(tileSizes.size() >= op.getNumLoops()) ?

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
174–176

Remove trivial braces

187–188

I think we don't need to pass LinalgTilingOptions() because that is the default constructor?

This revision now requires changes to proceed.Nov 12 2020, 12:52 AM
mravishankar marked 10 inline comments as done.

Make the tileAndFuse only tile the fusable loops. Also address comments.

Make Tile+Fuse only tile the fusable loops and leave the unfused loops
as is. The TileAndFuse pattern does the tiling of the unfused
operations.
Address other comments.

antiagainst added inline comments.Nov 14 2020, 4:49 AM
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
139

tiledLastLinalgOp could be a more clear name with self-documentation.

143

These are the newly generated ops that inside the fused loops? Would be nice to be clear.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
227

No need to have helper function to given it does not provide much additional value.

547

Is it necessary to copy the dependence struct here?

793

auto tileSizes = llvm::to_vecor<4>(tileSizeVector)?

892

I think renaming this function as collectTilableAndFusableLoops would be clearer and read better.

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
168

We don't care about the order here? (producers is only used at L182 for count). A normal SmallSet will do?

187

This sentence does not read properly for me.

202

This constant op can be created outside the for loop?

hanchung added inline comments.Nov 16 2020, 8:44 AM
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
592–597

lastFusableLoops should update for each iteration. Otherwise, it only checks if pos < the first fusable loop. E.g.,

(d1, d2, d3) -> (d1, d3, d2)

If all the loops are fusable, the lastFusableLoops is 0 forever, and we won't catch this case.

596

I would use < because they should be identical?

662–665

drop trivial braces

723–724

nit: I think we can use auto here because llvm::seq<xxx> states the type.

795–798

I think LLVM style doesn't want braces for this. See an example in https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

if (isa<VarDecl>(D)) {
  for (auto *A : D.attrs())
    if (shouldProcessAttr(A))
      handleAttr(A);
}

The inner for-body is not wrapped with braces.

hanchung requested changes to this revision.Nov 16 2020, 8:48 AM
This revision now requires changes to proceed.Nov 16 2020, 8:48 AM
mravishankar marked 10 inline comments as done.

Address comments and rebase

mravishankar added inline comments.Nov 18 2020, 4:00 PM
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
139

This is consistent with the TiledLinalgOp described above. I am inclined to leave as is.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
261

Ack. I was confused when I saw it second time (before I had that longer name).

547

Just copying the LinalgDependenceGraphElem and not the entire DependenceGraph. I think the convention is to pass by value.

588–593

Thanks for catching this. Indeed the logic was flawed. Updated with the fix.

592–597

Ah you are right. Thanks for catching that as well. Changed.

596

It could be identical (the transpose name might mislead here, but I dont know of a better name)

You could have an `affine_namp<(d0, d1, d2) -> (d1, d1, d1)>

Still might be fusable, but havent tested it.

662–665

Spanning a couple of lines. I think the convention is to still keep it.

686

Added some more comments on the todo.

793

I think its the same really.

842

This part is removed now.

892

Just made it collectFusableLoops.

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
187

Removed the sentence. It is not needed anymore

187–188

Not needed anymore

asaadaldien accepted this revision.Nov 18 2020, 4:55 PM
asaadaldien added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
229

Note: This is an example of what I was talking about last week =). If we have the computation that is used to compute loop bounds as part of the IR of being computed as part of the Linalg-> Loops transformation things like that will be easier.

antiagainst accepted this revision.Nov 19 2020, 5:22 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 19 2020, 7:03 PM
This revision was automatically updated to reflect the committed changes.
mravishankar reopened this revision.Nov 23 2020, 9:11 AM

Reopening since the commit was reverted.

Updating the change with https://reviews.llvm.org/D91503 to address some failures.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 23 2020, 10:31 AM
This revision was automatically updated to reflect the committed changes.