Page MenuHomePhabricator

[mlir][memref] Add `memref.elt_bitcast` op
Needs ReviewPublic

Authored by Hardcode84 on Nov 4 2022, 4:19 PM.

Details

Summary

This op allows to reinterpret between memrefs with different element types.
See the discussion https://discourse.llvm.org/t/rfc-memref-bitcasting/66395

Diff Detail

Event Timeline

Hardcode84 created this revision.Nov 4 2022, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 4:19 PM
Hardcode84 requested review of this revision.Nov 4 2022, 4:19 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
2134

This can be marked MemRefsNormalizable as well.

2138

an existing memref ... as a ...

2138

with a different ...

2139

have the same shape, ...

2140

-> The source memref should be allocated with ...

2141

with the proper

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
3167–3168

Can use auto here since the type is on the RHS.

3170–3174

Nit: Messages can all start in lowercase. No need of full stop at the end since there would be a trailing part.

mlir/test/Dialect/MemRef/canonicalize.mlir
852–854
853–854

Nit: Indent the start of the actual MLIR fragment correctly.

bondhugula requested changes to this revision.Nov 4 2022, 7:44 PM
bondhugula added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
3176–3180

This check looks incomplete - it doesn't cover vector elemental type cases?

This revision now requires changes to proceed.Nov 4 2022, 7:44 PM
Hardcode84 updated this revision to Diff 473420.Nov 5 2022, 5:27 AM

review comments, better folder

Hardcode84 marked 10 inline comments as done.Nov 5 2022, 5:29 AM
Hardcode84 added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
3176–3180

Checking vector and index sizes require DataLayout, I'm not sure if we should do this in verifier.

bondhugula added inline comments.Nov 5 2022, 8:21 AM
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
3176–3180

Can elt type bitcasts like those from vector<2xi16> to <i16> or to <i64> still be valid for some data layout? (We don't need to worry about index types here. memrefs of index type may not even be a case of interest for such elt type bit casts.)

Hardcode84 added a comment.EditedNov 8 2022, 2:39 AM

Can elt type bitcasts like those from vector<2xi16> to <i16> or to <i64> still be valid for some data layout?

More realistic case will be vector<3xi32> <-> vector<2xi64> cast, where vector<3xi32> may or may not be padded to 128 bits depending on layout.
Default data layout implementation assumes it is https://github.com/llvm/llvm-project/blob/8f60eee9144cd4178938d231ecccb65c43f78cde/mlir/lib/Interfaces/DataLayoutInterfaces.cpp#L78, but it can be overridden.
And I don't want to replicate this vector logic in verifier anyway. So the question is, is it ok to access data layout structures in op verifier?

And another question, do we really need such strict check in verifier? IMO, it better to do a trivial int/float check in verifier and then potentially reject it on memref->llvm conversion level where we have all required info about layout and which seems a proper place for this.

Dismissing change request.

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
3191–3196

Code comment needed here and/or above.

Hardcode84 updated this revision to Diff 474015.Nov 8 2022, 8:27 AM

rebase, add comment

Hardcode84 marked an inline comment as done.Nov 8 2022, 8:29 AM