Page MenuHomePhabricator

[mlir] Add MemRefReinterpretCastOp definition to Standard.
ClosedPublic

Authored by pifon2a on Oct 19 2020, 11:39 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Oct 19 2020, 11:39 AM
pifon2a requested review of this revision.Oct 19 2020, 11:39 AM

Can you link the discourse discussion on this? Seems really weird that we have many seemingly overlapping cast ops.

pifon2a edited the summary of this revision. (Show Details)Oct 19 2020, 11:45 AM

@rriddle Right, I forgot. This is the first PR to implement https://llvm.discourse.group/t/rfc-standard-memref-cast-ops/1454/15

rriddle requested changes to this revision.Oct 19 2020, 11:48 AM
rriddle added inline comments.
mlir/include/mlir/IR/StandardTypes.h
470 ↗(On Diff #299115)

In what way is this different from ShapedType::getElementType?

This revision now requires changes to proceed.Oct 19 2020, 11:48 AM

@rriddle Right, I forgot. This is the first PR to implement https://llvm.discourse.group/t/rfc-standard-memref-cast-ops/1454/15

Thanks! Appreciate it.

pifon2a updated this revision to Diff 299124.Oct 19 2020, 11:55 AM

Cast to ShapedType to get element type.

pifon2a added inline comments.Oct 19 2020, 11:57 AM
mlir/include/mlir/IR/StandardTypes.h
470 ↗(On Diff #299115)

Done. Now I am casting twice: once to use getElementType and also to use getMemorySpace. The fact that unranked memref is a ShapedType is a bit confusing.

Do you want to add a to in the syntax?

memref_reinterpret_cast %in to offset: ..., sizes: ...

Otherwise, the proposed syntax is far more readable than subview which is missing offset, sizes, and strides markers and prints three integer arrays straight even in its custom form - in fact, its long form is more readable than its custom form!

bondhugula requested changes to this revision.Oct 19 2020, 8:13 PM
bondhugula added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2238

Do you have a stray ] here or are you missing the opening one? You have more closing brackets than opening ones in the syntax.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
276

Nit: '['

355

The comment appears out of place - assumedArrayAttr?

364

Missing doc comment.

2272

It's useful to have the comment on this include an example for ready reference locally on the custom form syntax.

2293

Likewise here.

2364

Comment here - to distinguish with what the code further below is doing.

2390

Short comment.

2401

Likewise.

This revision now requires changes to proceed.Oct 19 2020, 8:13 PM
pifon2a updated this revision to Diff 299294.Oct 20 2020, 1:29 AM
pifon2a marked 9 inline comments as done.

Address the comments.

pifon2a updated this revision to Diff 299609.Oct 21 2020, 2:35 AM

Fix the tests.

herhut requested changes to this revision.Oct 21 2020, 6:28 AM

Nice, thanks!

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2250

Why is this plural? I though this allows a single offset? Is this to enable reuse in verification and parsing? I would rather specify this with Optional here and add some glue at the use sites. Or do you expect more users to prefer the range/array representation?

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
357

nit: of -> or

2370

cast to BaseMemrefType here?

This revision now requires changes to proceed.Oct 21 2020, 6:28 AM
ftynse added inline comments.Oct 21 2020, 8:55 AM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
318

I think the ParseResult parseAttribute(Attribute &result, Type type = {}) version should work here without needing the placeholder

322

Nit: isa followed by cast is an anti-pattern

2309

Can this be expressed with declarative assembly? It looks like if you define functions that forward the right name and dynamic marker to printListOfOperandsOrIntegers, you could use custom directives.

pifon2a updated this revision to Diff 299782.Oct 21 2020, 12:41 PM
pifon2a marked 6 inline comments as done.

Address the comments.

@herhut, @ftynse Thanks!

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2250

Yes, this is for verification and parsing and because I inherit BaseOpWithOffsetSizesAndStrides (mostly because of that).

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
318

i ended up using this one:

template <typename AttrType>
  ParseResult parseAttribute(AttrType &result, Type type = {}) {
322

yeah, just moving code always leads to cleaning it up.

357

it's array<int>. I think "of" is correct here.

2309

Good point. That would lead to changing SubViewOp et al. and a pretty large refactoring. It should be a separate PR.

herhut accepted this revision.Oct 22 2020, 5:07 AM
herhut added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2375

This cast should be no longer needed.

pifon2a updated this revision to Diff 299940.Oct 22 2020, 6:01 AM

Rebase and address the comment

This revision was not accepted when it landed; it landed in state Needs Review.Oct 22 2020, 6:18 AM
This revision was automatically updated to reflect the committed changes.