Page MenuHomePhabricator

[mlir] Add a new builtin `unrealized_conversion_cast` operation
ClosedPublic

Authored by rriddle on Jan 15 2021, 2:48 PM.

Details

Summary

An unrealized_conversion_cast operation represents an unrealized conversion
from one set of types to another, that is used to enable the inter-mixing of
different type systems. This operation should not be attributed any special
representational or execution semantics, and is generally only intended to be
used to satisfy the temporary intermixing of type systems during the conversion
of one type system to another.

This operation was discussed in the following RFC(and ODM):

https://llvm.discourse.group/t/open-meeting-1-14-dialect-conversion-and-type-conversion-the-question-of-cast-operations/

Depends On D94831

Diff Detail

Event Timeline

rriddle created this revision.Jan 15 2021, 2:48 PM
rriddle requested review of this revision.Jan 15 2021, 2:48 PM
lattner accepted this revision.Jan 15 2021, 5:37 PM
lattner added a subscriber: lattner.

Looking forward to this!!

mlir/include/mlir/IR/BuiltinOps.td
229

bikesheding the name, "partial_conversion_cast" has two things going on that are confusing dialect "conversion" and value "cast"ing.

WDYT about just "partial_convert" or "conversion_cast" or something like that?

231

Should this also be marked as having no side effects?

248

What is the semantic of a zero operand cast, is this a constant of some sort? Is this an undef/poison like thing?

I'm not sure what the use-case is here, but if there is one then it should be defined a bit more I think.

254

I know these are just examples, but I'd use something a bit more obviously different so it is clear it is a dialect lowering thing, e.g.:

%results:2 = partial_conversion_cast(%operand : !ast.enum) to !midlevel.selector, !midlevel.payload

mlir/lib/IR/BuiltinDialect.cpp
249

Stylistic thing, but you could define the class inside the getCanonicalizationPatterns function to scope it better.

256

Marking as no side effect would eliminate the need for this.

273

Have you considered inlining this into its own caller? I don't see a reason for the extra overhead.

This revision is now accepted and ready to land.Jan 15 2021, 5:37 PM
silvas added a subscriber: silvas.Jan 15 2021, 5:41 PM
silvas added inline comments.
mlir/include/mlir/IR/BuiltinOps.td
251

This is pretty non-standard syntax. Why not

partial_conversion_cast : () -> f64
partial_conversion_cast %operand : i32 -> f64
partial_conversion_cast %operand : i32 -> (i32, i32)
partial_conversion_cast %operand, %operand : (i32, i32) -> i32

My fingers and eye-parsers have that syntax memorized from countless other ops, and the current syntax is no more succinct.

mlir/lib/IR/BuiltinDialect.cpp
247

nit: space before (

jpienaar added inline comments.
mlir/include/mlir/IR/BuiltinOps.td
229

transient_cast ?

232

"An operation" doesn't add much

232

What does opaquely refer to here? E.g., how is it more opaque than a regular cast?

251

+1

silvas added inline comments.Jan 15 2021, 5:47 PM
mlir/include/mlir/IR/BuiltinOps.td
229

I think that jdd's unrealized_type_conversion is also a strong contender. I think that captures the intent really well: "there is further type conversion that needs to happen somewhere, but hasn't happened yet".

Or to use Mehdi's terminology, this op is a "promise" that further type conversions are coming later in the pipeline that will make it unnecessary.

rriddle retitled this revision from [mlir] Add a new builtin `partial_conversion_cast` operation to [mlir] Add a new builtin `unrealized_conversion_cast` operation.Jan 15 2021, 6:43 PM
rriddle edited the summary of this revision. (Show Details)
rriddle updated this revision to Diff 317142.Jan 15 2021, 6:43 PM
rriddle marked 11 inline comments as done.

Address most comments

mlir/include/mlir/IR/BuiltinOps.td
229

Went with UnrealizedConversionCast, given that I think it bridges both worlds. (Glad I didn't have to think of a name myself.)

231

The mindset I have is that we should ideally treat this operation as conservatively as possible. Marking this as no side effecting could be lossy depending on what the conversion at the end is intended to be, given that if an operation is non side effecting it can be CSEd/moved as part of code motion/have things trivially moved around it/etc. My view of this op is that things should basically treat it as an unregistered operation/unknown call, and not make assumptions about it.

248

The use case I had in mind is that during conversion you can end up with a 1->0 conversions (most often during function signature conversion); i.e. where a type gets dropped. If uses of the original value still linger around, we have to insert a cast operation of some kind to temporarily materialize a value of the desired type. This fits in the partial lowering use cases, e.g. where you may want to lower function signatures in a ModulePass and then function bodies in a followup FunctionPass.

251

This is the standard syntax for cast operations upstream (for a few years now). I'd rather be consistent (and I also have a slight preference for this style ;)

mlir/lib/IR/BuiltinDialect.cpp
249

Reminds me that I should revive my old patch of being able to construct patterns inline when adding them to the rewriter.

273

That is how I originally had it, but it felt slightly weird. Moved it back.

lattner accepted this revision.Jan 15 2021, 10:27 PM
lattner added inline comments.
mlir/include/mlir/IR/BuiltinOps.td
231

Ok, but you can't have it both ways. If it has side effects, you should drop the canonicalization that removes unused casts.

I don't really understand this design point though. it is the result of a partial conversion. These things are doomed to go away. How can they have side effecting semantics? Can you give an example that would illustrate this? If you can, does it need to be handled by this instead of some other dialect specific op?

248

Makes sense, how about making the example something that would show this then, e.g.:

%result = partial_conversion_cast to !mylanguage.tuple<>

and mention explicitly that this can be useful for things where you can have empty tuples.

rriddle added inline comments.Jan 15 2021, 10:32 PM
mlir/include/mlir/IR/BuiltinOps.td
231

Yeah, I don't have a strong motivating example. I'm okay with marking it as non-side effecting and letting users that really need to preserve something add their own proper cast operation, which is probably what they should be doing in those cases anyways.

+1, thanks River

herhut added a subscriber: herhut.Jan 18 2021, 12:47 AM

I don't mind adding this generic default cast but I hope dialect conversion will remain configurable as to which cast is being used.

mlir/include/mlir/IR/BuiltinOps.td
231

I would be in favour of marking it nosideeffect, as well. After all, these casts often just disappear and I always found it odd that dialect conversion might make them disappear if they are unused but if that did not happen, dcr would not. Also, there is very little guarantee where they get inserted in the first place (unless we want to specify this and nail it down), so relying on the casts exact placement seems dangerous.

251

Wouldn't that be partial_cast <operands> : <srctypes> to <dsttypes>?

partial_cast : () to (t1, t2)
partial_cast %v0 : t0 to t1
partial_cast %v0 : t0 to (t1, t2)
partial_cast %v0, %v1 : (t0, t1) to (t2)

I don't mind either syntax but if consistency is the argument, it should be close to what we have.

rriddle updated this revision to Diff 317659.Jan 19 2021, 12:42 PM
rriddle marked 7 inline comments as done.

Address feedback