Please fix the comment.

I like the new syntax and encoding! Great cleanup. Some more verification of valid forms would be nice.

Nice!

Thanks!

Some first comments.

I will do the change to use ShapedTypeComponents in a follow up to enable reuse for the memref case.

Comments

I have considered to place a templated version of joinShapes somewhere (to be used with ShapedType and a template parameter to figure out the type to create) but wasn't sure where and whether useful.

More comments

Comments...

Thanks!

Neat!

Thanks!

Looks good with comments fixed. Thanks!

Also, as a nit, you could rephrase the summary to just read

Thanks. Please submit a corresponding PR for TensorFlow before landing.

Can you be a bit more descriptive in the summary of what bug was fixed? If the bug was filed somewhere, reference it. Otherwise please describe a little more.

Do you intend to remove the BufferAssignmentPlacer itself in a separate step?

Thanks for cleaning this up!

Mostly nits. Thanks for adding this!

Thanks. Please fix comments before landing.

In D85634#2207669, @rriddle wrote:

In D85634#2207661, @silvas wrote:

Is there anything about this specific to index type? Can this issue happen with any user-defined type?

The solution in this revision looks like a hack. Returning the input when a materialization is requested defeats the whole purpose of materializations, and only works because the assert isn't turned on yet. Realistically, the underlying issue can happen with any type right now. I have a proper fix in the pipeline that should solve this case, where the conversion infra removes casts that become redundant.

In D85634#2208550, @mehdi_amini wrote:

The simultaneous application of patterns from SCF to standard and from standard to LLVM rewrites the following to invalid IR.

Do we actually need a simultaneous application?

Thanks!

Thanks for cleaning this up!

Thanks for clarifying!

Nice, some nits.

Just some quick comments.

Thanks!

It is a bit sad the we have two dynamic dispatch implementations but I guess there is no way around it. A LLVMTypeNew::translate overloaded method would be great :)

Looks good. memref<index> has landed upstream so please fix before landing.

Thank you!

Thanks. With one more test this is good to land!

Looks reasonable to me. However, we are registering this for a dialect that is in very local use. Maybe @rriddle can advise.

Please fix linter error and wait for @ftynse to comment.

Thanks for fixing this!

Fix test

Rebase

Comments

Thanks!

Comments on formatting

Thanks. I only have a minor nit.

Maybe mark it as NFC. Looks good with @bondhugula's comment addressed.

Thanks.

Adding the op SGTM.

In D83799#2156321, @bondhugula wrote:

What you say is completely doable and meaningful as a rewrite in HLO, but I think you are assuming that eliminating such tuples completely is always desirable or possible before applying buffer conversion. On a side note, the real source of such return value tuples in TF/MLIR context is -tf-promote-resources-to-args, which creates a single tuple value for all TF outputs and resource var output aliases. The handling of the tuples itself is about 6-7 lines of code here, but there is a mix of concerns (as you suggest) if one assumes the tuples could always be eliminated and it'd be desirable to do so. But that's a big assumption because such tuples may only ultimately go away while we do the tensor to buffer conversion (where return value to output arg is happening). I have a conversion pattern on the HLO side that deals with xla_hlo.tuple (now mhlo.tuple) pretty much doing what the test dialect tensor tuple op is doing here. A tuple tensor being returned could potentially be an operand for another intermediate op for example. How do we remove tuples on the tensor op form without breaking op semantics?

We should discuss whether we in the long run want to allow known-size tensors for known-rank inputs.