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
196–200

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
196–200

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
205–207

nit drop braces for simple single-statement body of for

234–239

drop trivial braces

262

[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.

547
550

ditto

578–580

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?)

587

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).

587–592

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.

690

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?

850

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
120

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

124

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

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

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

546

Is it necessary to copy the dependence struct here?

801

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

874

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.

203

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
591–596

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.

595

I would use < because they should be identical?

666–669

drop trivial braces

731–732

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

803–806

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
120

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

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

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

546

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

587–592

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

591–596

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

595

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.

666–669

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

690

Added some more comments on the todo.

801

I think its the same really.

850

This part is removed now.

874

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
230

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.