Page MenuHomePhabricator

[mlir:OpConversionPattern] Add overloads for taking an Adaptor instead of ArrayRef
ClosedPublic

Authored by rriddle on Wed, Sep 22, 3:14 PM.

Details

Summary

This has been a TODO for a long time, and it brings about many advantages (namely nice accessors, and less fragile code). The existing overloads that accept ArrayRef are now treated as deprecated and will be removed in a followup (after a small grace period). Most of the upstream MLIR usages have been fixed by this commit, the rest will be handled in a followup.

Diff Detail

Event Timeline

rriddle created this revision.Wed, Sep 22, 3:14 PM
rriddle requested review of this revision.Wed, Sep 22, 3:14 PM
ftynse accepted this revision.Thu, Sep 23, 12:55 AM

Very nice usability improvement!

mlir/docs/Bufferization.md
146

Nit: I know this is a large-scale refactoring, but could we fix this particular place in the documentation to be adaptor.source() instead of using getOperands + magic number? For the sake of documenting best practices.

mlir/include/mlir/Conversion/LLVMCommon/Pattern.h
161

Is it too early to put LLVM_ATTRIBUTE_DEPRECATED here?

This revision is now accepted and ready to land.Thu, Sep 23, 12:55 AM
rriddle updated this revision to Diff 374649.Thu, Sep 23, 12:45 PM
rriddle marked 2 inline comments as done.

Update

aartbik added inline comments.Thu, Sep 23, 12:59 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
402–407

heads up: I added an emitCInterface parameter to some of these calls at exactly the same line, so you probably need to resolve some conflicts on the rebase with main

Thanks for the review Alex!

(Also thanks for the heads up Aart!)

mlir/include/mlir/Conversion/LLVMCommon/Pattern.h
161

Yeah, I think so. I'm going to send out the PSA first and then mark deprecated sometime early next week.

antiagainst accepted this revision.Thu, Sep 23, 3:23 PM

This is nice!

This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.