This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Vector][Transforms] Improve the control over individual vector lowerings and transforms
ClosedPublic

Authored by ftynse on Mar 23 2023, 1:27 PM.

Details

Summary

This revision adds vector transform operations that allow us to better inspect the composition
of various lowerings that were previously very opaque.

This commit is NFC in that it does not change patterns beyond adding rewriter.notifyFailure messages
and it does not change the tests beyond breaking them into pieces and using transforms instead of
throwaway opaque test passes.

Diff Detail

Event Timeline

Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache requested review of this revision.Mar 23 2023, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 1:27 PM

Drop one-off test pass option for multi-reduction lowering.

Get rid of TestVectorContractionLowering

nicolasvasilache retitled this revision from [mlir][Vector][Transforms] Improve the contol over individual vector lowerings and transforms to [mlir][Vector][Transforms] Improve the control over individual vector lowerings and transforms.

Get rid of TestVectorTransposeLowering

Revert spurious formatting changes

@ftynse @qcolombet what is the magic incantation to auto-declare produced dialects without having to declare dependent dialects ?

Having issues with the llvm.inline_asm part

Add declareDependentDialect<LLVM::LLVMDialect>(); and fix test

Reorder transforms alphabetically.

Add notifications

Get rid of TestVectorTransferLoweringPatterns

Get rid of TestVectorTransferFullPartialSplitPatterns

Drop untested transforms for now

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

Update commit message

springerm added inline comments.Mar 24 2023, 2:36 AM
mlir/lib/Dialect/Vector/TransformOps/VectorTransformOps.cpp
46

You can rewrite this as follows to support non isolated from above targets.

// Manually gather list of ops because the other GreedyPatternRewriteDriver
// overloads only accepts ops that are isolated from above.
SmallVector<Operation *> ops;
target->walk([&](Operation *nestedOp) {
  if (target != nestedOp) ops.push_back(nestedOp);
});
LogicalResult result =
    applyOpPatternsAndFold(ops, std::move(patterns));
46

Is this guaranteed to only replace/delete the target op? Otherwise, we have to attach a listener here. Same for all the other applyPatternsAndFoldGreedily uses in this file.

ftynse added inline comments.Mar 24 2023, 3:38 AM
mlir/lib/Dialect/Vector/TransformOps/VectorTransformOps.cpp
99–119

Could we factor out the common implementation and have it take a function_ref<void (RewritePatternSet &)> populatePatternCallabck? Maybe even turn this into a trait that we attach at the ODS level.

169

This probably needs to be declareGeneratedDialect, the transform IR doesn't use LLVM types. Same may be true for the vector dialect.

springerm accepted this revision.Mar 24 2023, 4:33 AM
springerm added inline comments.
mlir/lib/Dialect/Vector/TransformOps/VectorTransformOps.cpp
46

Since these ops have the FunctionStyle trait (all existing handles are invalidated), as long as we only modify IsolatedFromAbove op contents, this should be safe.

This revision is now accepted and ready to land.Mar 24 2023, 4:33 AM
nicolasvasilache marked 3 inline comments as done.Mar 24 2023, 4:34 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Vector/TransformOps/VectorTransformOps.cpp
46

Is this guaranteed to only replace/delete the target op? Otherwise, we have to attach a listener here. Same for all the other applyPatternsAndFoldGreedily uses in this file.

I expect that the FunctionalStyleTransformOpTrait invalidates all nested handles.
The listener will be indeed useful in the future when we want to improve usage and avoid always invalidating the world below.

@ftynse could you please confirm this makes sense to you ?

46

You can rewrite this as follows to support non isolated from above targets.

Yes, I would prefer to keep this for a followup though if you don't mind, this is already big enough and dropping these pesky test transforms is already a sizeable step forward.

169

ah yes, thanks! I keep on tripping myself over this ..

ftynse accepted this revision.Mar 24 2023, 4:50 AM
ftynse added inline comments.
mlir/lib/Dialect/Vector/TransformOps/VectorTransformOps.cpp
46

I expect that the FunctionalStyleTransformOpTrait invalidates all nested handles.
The listener will be indeed useful in the future when we want to improve usage and avoid always invalidating the world below.

@ftynse could you please confirm this makes sense to you ?

Yep.

Since these ops have the FunctionStyle trait (all existing handles are invalidated),

It's not all existing handles, but all handles to ops nested within the payload op associated with the operand (consumed) handle.

ftynse commandeered this revision.Mar 24 2023, 5:39 AM
ftynse edited reviewers, added: nicolasvasilache; removed: ftynse.

Will do some deduplication here.

ftynse updated this revision to Diff 508066.Mar 24 2023, 6:20 AM
ftynse marked 2 inline comments as done.

Remove code repetition by turning pattern-based applyToEach into a trait.

nicolasvasilache accepted this revision.Mar 24 2023, 6:27 AM

Thanks much!

mlir/lib/Dialect/Vector/TransformOps/VectorTransformOps.cpp
46

ack

This revision was landed with ongoing or failed builds.Mar 24 2023, 7:01 AM
This revision was automatically updated to reflect the committed changes.
mlir/test/Dialect/Vector/vector-contract-to-parallel-arith-transforms.mlir