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

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

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–195

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

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

mlir/include/mlir/Dialect/Linalg/Passes.td
40

Moved to test.

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

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
201–203

nit drop braces for simple single-statement body of for

230–235

drop trivial braces

258

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

560
563

ditto

591–593

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

600

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

600–605

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.

647

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?

823

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
138

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

142

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

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

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

559

Is it necessary to copy the dependence struct here?

774

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

872

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
604–609

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.

608

I would use < because they should be identical?

623–626

drop trivial braces

704–705

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

776–779

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
138

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

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

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

559

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

600–605

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

604–609

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

608

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.

623–626

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

647

Added some more comments on the todo.

774

I think its the same really.

823

This part is removed now.

872

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
226

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.