Page MenuHomePhabricator

[mlir] Make OpBuilder::createOperation to accept raw inputs
ClosedPublic

Authored by Chia-hungDuan on Mar 1 2022, 5:03 PM.

Details

Summary

This provides a way to create an operation without manipulating
OperationState directly. This is useful for creating unregistered ops.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Mar 1 2022, 5:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 5:03 PM
Chia-hungDuan requested review of this revision.Mar 1 2022, 5:03 PM
jpienaar added inline comments.Mar 1 2022, 8:06 PM
mlir/test/lib/Dialect/Test/TestDialect.cpp
826–827

Is SmallVector / temporary needed or is this for readability? (e.g., could we use {lhs, rhs} below instead of innerOperands and let ValueRange's constructor take care of it).

Chia-hungDuan added inline comments.Mar 1 2022, 9:35 PM
mlir/test/lib/Dialect/Test/TestDialect.cpp
826–827

Yes, it just for readability. Or i would use /*operands=*/{lhs, rhs}, either way should be fine.

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

mlir/test/lib/Dialect/Test/TestDialect.cpp
826–827

I would rather inline it. As an aside, if we want to temp variable, we should use an array instead of SmallVector.

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

OperationState is more like an implementation detail thing and having this makes the preparation of creating operation the same in between registered and unregistered op. I mean, when a user wants to create an unregistered op, they don't need to know there's a OperationState structure to work with. They could have same experience of creating operation. So far, I'm not sure if there's anything bad if we provide this.

I think we can make the createOperation(OperationState) private and having create(...) as public. Or do you think it's better to have both of them for users? I.e., have both create(OperationState) and create(...) in public?

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

OperationState is more like an implementation detail thing and having this makes the preparation of creating operation the same in between registered and unregistered op. I mean, when a user wants to create an unregistered op, they don't need to know there's a OperationState structure to work with. They could have same experience of creating operation. So far, I'm not sure if there's anything bad if we provide this.

I think we can make the createOperation(OperationState) private and having create(...) as public. Or do you think it's better to have both of them for users? I.e., have both create(OperationState) and create(...) in public?

OperationState isn't really just an implementation detail though, given that it is heavily exposed (e.g. all builders use it, parsers use it, etc.).

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

OperationState is more like an implementation detail thing and having this makes the preparation of creating operation the same in between registered and unregistered op. I mean, when a user wants to create an unregistered op, they don't need to know there's a OperationState structure to work with. They could have same experience of creating operation. So far, I'm not sure if there's anything bad if we provide this.

I think we can make the createOperation(OperationState) private and having create(...) as public. Or do you think it's better to have both of them for users? I.e., have both create(OperationState) and create(...) in public?

OperationState isn't really just an implementation detail though, given that it is heavily exposed (e.g. all builders use it, parsers use it, etc.).

Sorry, I didn't say it clearly. For user's perspective, writing pass or patterns, they don't need to know OperationState if they only work on registered op.

mehdi_amini accepted this revision.Mar 3 2022, 2:58 AM

LGTM: it is nicer to write than the having to explicitly wrap into an OperationState.

But please remove the SmallVector!
(and wait for River's approval as well)

mlir/include/mlir/IR/Builders.h
418

I would take a StringAttr instead of a StringRef: it encourages a more efficient API

This revision is now accepted and ready to land.Mar 3 2022, 2:58 AM
Chia-hungDuan marked an inline comment as done.

Use StringAttr instead and updated some cases to use the create without OperationState

Also changed the createOperation to create.

After checking some cases, OperationState is useful when building some complicated cases. For example, they don't need to allocate individual containers for each argument. So I think having both of them will be better.

rriddle accepted this revision.Mar 10 2022, 1:13 PM
This revision was landed with ongoing or failed builds.Mar 23 2022, 3:17 PM
This revision was automatically updated to reflect the committed changes.