Page MenuHomePhabricator

[mlir][MemRef] Introduce a memref.extract_metadata op.
ClosedPublic

Authored by nicolasvasilache on Aug 19 2022, 8:42 AM.

Details

Summary

This is the counterpart of memref.reinterpret_cast and is useful to lift
strided memref manipulation out of the LLVM dialect.

Discussion: https://discourse.llvm.org/t/extracting-dynamic-offsets-strides-from-memref/64170

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Aug 19 2022, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2022, 8:42 AM
stellaraccident accepted this revision.Aug 19 2022, 8:49 AM

This looks good to me but I'd like to see the reviews of the other contributors to the conversation before landing.

mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
705

Nit: it seems that it can be used anywhere (early or late) but it is resolved late?

This revision is now accepted and ready to land.Aug 19 2022, 8:49 AM
mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
705

Could you please elaborate what "resolved" means ?
I'm thinking it will fold away in various ways or continue to get materialized when going to LLVM (if uses remain).

mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
705

My word "resolved" is likely the same as your word "folded" (but per the discussion on discourse, I think that such folding needs to be explicit as it has phase ordering constraints).

I was focusing more on "used": there really is not a constraint on where it gets used in various settings.

Op itself is LGTM but maybe we should also add some simple test in ops.mlir?

Thanks

bondhugula added inline comments.
mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
705

Queries -> Extracts

705

I'd not mix commentary on usages in the pipeline early or late in the first para. Please move this to later if it all it's retained.

724–725

This can be brought up instead.

730

Missing memref..

757

Test cases are missing.

bondhugula requested changes to this revision.Aug 21 2022, 7:50 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
703

Recommend avoiding "late/early" in the summary - that's all application-specific and relative. Instead,

Extracts a buffer base with offset and strides

?

705

Behavior of the op when such stride info can't be extracted has been left undocumented here.

730

extract_metadata would be too generic a name and not reflect what this op is returning. Instead, extract_stride_metadata or extract_stride_info looks appropriate.

This revision now requires changes to proceed.Aug 21 2022, 7:50 AM
nicolasvasilache marked 9 inline comments as done.

Address comments.

nicolasvasilache marked an inline comment as done.Aug 24 2022, 7:07 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
703

SGTM, thanks!

705

Opted for removing this as @bondhugula suggested.

I think the intended use case section is detailed enough.

730

went for memref.extract_strided_metadata

bondhugula accepted this revision.Aug 24 2022, 3:01 PM

LGTM - but some comments to rephrase/rewrite certain things.

mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
708–709
716–728

I think these paras need some rephrasing - the first bullet refers to LLVM lowering, but the header talks about ops needing to manipulate stride info outside the LLVM dialect. I think this is a bullet on the latter is missing.

Also, I think "... to lift the materialization of the internal logic of ops that manipulate metadata" should be rewritten better.

Beyond these, I see this extraction needed for completeness sake just like we have dim op: you can have a memref passed to a function and extracting strides, sizes etc. may be needed to subsequently say construct a new memref -- let's say with slightly different strides, sizes but based on the previous memref.

717

metadata -> memref metadata?

725

"next problem" is unclear here.

733

Syntax broken here: sizes:2, ...

This revision is now accepted and ready to land.Aug 24 2022, 3:01 PM
nicolasvasilache marked 5 inline comments as done.

Address

mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
716–728

Rephrased and reshuffled a bit more, feel free to make another unreviewed pass over this and just land something that is more to your liking if this does not yet fit the bill.

Thanks!

This revision was landed with ongoing or failed builds.Aug 26 2022, 9:09 AM
This revision was automatically updated to reflect the committed changes.