Cleanup ConvertToStream to accomodate llvm/llvm-project@3f136f7

[MaheshRavishankar]

The upstream change llvm/llvm-project@3f136f7 allows ConvertToStream to better handle the 1:N type conversion, specifically the type conversion of a tensor<...> to !stream.resource<*>, index. Now instead of trying to work around builtin.unrealized_conversion_casts the conversion can get the converted values directly using the OneToNAdaptor and can also replace a tensor<..> directly with multiple values using the ConversionPatternRewriter::replaceOpWithMultiple.

These changes are required to drop the revert of llvm/llvm-project#116470 in the IREE ToM. The change drops these reverts as well.

Fixes #19448

[MaheshRavishankar]

Took some debugging but I think this is pretty close to the end state that we want here. @benvanik PTAL for any red flags you see. There is still some cleanup I need to do here to remove things that might be unnecessary.

@matthias-springer overall kudos on the changes to Dialect Conversion! At least in this case the changes you have made get things aligned better.

The only thing I will say though is the use of ArrayRef<ValueRange> in replaceOpWithMultiple is a bit of a footgun/pain. Both of these are reference objects, so you need to create something explicit to hold the values. So I had to do something like this https://github.com/iree-org/iree/pull/19451/files#diff-98ccf8581ef8de4f0a02f059079dbb1dbcbe3d2d398c69360cc5facc5795f608R83

[matthias-springer]

The only thing I will say though is the use of ArrayRef<ValueRange> in replaceOpWithMultiple is a bit of a footgun/pain. Both of these are reference objects, so you need to create something explicit to hold the values. So I had to do something like this https://github.com/iree-org/iree/pull/19451/files#diff-98ccf8581ef8de4f0a02f059079dbb1dbcbe3d2d398c69360cc5facc5795f608R83

I went through a few iterations for this API. No good solution so far, I had to write similar code... Originally, I wanted to have a single replaceOp function that supports 1:1 and 1:N, but that gets tricky because Value can be implicitly converted to ValueRange and ArrayRef<Value>. I gave up on that. Let me know if you have a better idea.

[MaheshRavishankar]

The only thing I will say though is the use of ArrayRef<ValueRange> in replaceOpWithMultiple is a bit of a footgun/pain. Both of these are reference objects, so you need to create something explicit to hold the values. So I had to do something like this https://github.com/iree-org/iree/pull/19451/files#diff-98ccf8581ef8de4f0a02f059079dbb1dbcbe3d2d398c69360cc5facc5795f608R83

I went through a few iterations for this API. No good solution so far, I had to write similar code... Originally, I wanted to have a single replaceOp function that supports 1:1 and 1:N, but that gets tricky because Value can be implicitly converted to ValueRange and ArrayRef<Value>. I gave up on that. Let me know if you have a better idea.

You can have the API be a ArrayRef<SmallVector<Value>> . That will avoid the reference of reference.

[matthias-springer]

Is there a particular reason why you said ArrayRef<SmallVector<Value>> and not SmallVector<ValueRange>? Is either one of those better?

[MaheshRavishankar]

Is there a particular reason why you said ArrayRef<SmallVector<Value>> and not SmallVector<ValueRange>? Is either one of those better?

SmallVector<ValueRange> is effectively array of pointers. So to populate that I will need to build a SmallVector<SmallVector<Value>> and then take reference for each of the inner SmallVector<Value> to build a SmallVector<ValueRange> . Also using SmallVector<ValueRange> would be passing it by value which is IMO a footgun cause you are passing an SmallVector of references by value and assuming that the underlying references stay valid for as long as it is used in the callee. If the callee for whatever reason decides to copy the SmallVector<ValueRange> it is just copying references to something that might very easily not be live anymore.

ArrayRef<SmallVector<Value>> is a reference to the outer SmallVector of SmallVector<SmallVector<Value>> . You are passing a reference to the container that owns all its data. That is much better I think and you can just populate the SmallVector<SmallVector<Value>> to start with and avoid creating new intermediate SmallVectors.