This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add a structured.pack_transpose transform op
ClosedPublic

Authored by nicolasvasilache on Jan 18 2023, 12:29 PM.

Details

Summary

This transform is complementary to the structured.pack op which
allows packing a whole op but does not allow transposes on the individual
operands.

structured.pack_transpose allows transposing single operands connected to
pack or unpack ops after the fact.

This makes the system overall more composable than e.g. a giant transform
op with all permutation specified at once.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jan 18 2023, 12:29 PM

Still needs tests but I'd appreciate early feedback on the logic and descriptions.

Bug fixes + add first test.

More bugfixes and add second test.

chelini added inline comments.Jan 19 2023, 12:35 AM
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1473

Mind improving this comment please? The comment reflects only the first few lines of this method. If this should not be part of the doc we can use //

1497

use //?

1532

debug leftover. Same below.

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3242

nit: unfinished comment.

nicolasvasilache marked 4 inline comments as done.

Address comments.

nicolasvasilache edited the summary of this revision. (Show Details)

Update commit message.

I didn't check the logic of transposition in great detail, deferring to @chelini for that.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
433–451

Do we expect concrete ops to ever reimplement this behavior? Otherwise, this looks like a good fit for extraSharedClassDeclaration instead of being an interface method. This lets the body live in C++ rather than string, allows for faster dispatch (no pseudo-vtable), and avoids funny discrepancies like the difference between $_op and this->getOperation().

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
457
498

Nit: this will generate getUnPackOp that would be inconsistent with getTargetPackOrUnpackOp (P not capitalized in "unpack).

508–512

Nit: you can just do DeclareOpInterfaceMethods<TransformOpInterface>` in the definition of the op class and this becomes unnecessary.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1196–1453

Missed it the last time, but this also needs transform::modifiesPayloadIR effect.

1462

This also needs a transform::modifiesPayloadIR.

1462–1466

Looks like this could have had a FunctionalStyleTransformOpTrait.

1517–1518

Maybe we don't need to pass it then?

1553

This moves the body from linalgOp leaving it in potentially invalid state. I see that linalgOp is still necessary at the callsite so it cannot be immediately deleted here, but this needs to be at least documented on the function. Better, store everything that's needed from linalgOp at the callsite before calling this and actually call replaceOp here.

1605–1607

Incorrect error message.

1797–1798

One day you'll fix your editor to tell it 80 cols are fine and there's no need to do 78 on irrelevant parts of the file...

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3261

The combination of name and type confused me. getInnerDimPos sounds like it returns one value (position), which would be treated as the size of the vector. It is not in fact the case, but looks confusing. Any clarification (comment, different code structure, different name) is welcome.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1797–1798

I've been at 81 for a while now.
I am utterly unclear why this would happen here and not in other places..

2075

why would it reflow here and not on the code on l2003 above is beyond me ..

ftynse added inline comments.Jan 19 2023, 4:20 AM
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1797–1798

Maybe some rouge clang-format pre-commit git hook?

nicolasvasilache marked 2 inline comments as done.Jan 19 2023, 4:34 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1797–1798

Sometimes I can comment / uncomment locally while debugging and things get auto-formatted and not reflowed back.
But this instance is still puzzling to me ..

nicolasvasilache marked 12 inline comments as done.Jan 19 2023, 5:56 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
498

went for UnPack to be consistent with tensor::UnPackOp

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1517–1518

I find the explicit chaining requirement to help with the legibility atm.
I'd prefer to remove later if/when we feel like its a burden.

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3261

I am not seeing this, maybe I applied some cleanups in the meantime ?

nicolasvasilache marked 3 inline comments as done.

Rebase and address comments.

Drop spurious formatting changes

chelini accepted this revision.Jan 20 2023, 12:19 AM

Looks good to me, thanks.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
1361

nit: UnPackOp

This revision is now accepted and ready to land.Jan 20 2023, 12:19 AM
This revision was landed with ongoing or failed builds.Jan 20 2023, 12:33 AM
This revision was automatically updated to reflect the committed changes.