This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add async.value type to Async dialect
ClosedPublic

Authored by ezhulenev on Sep 29 2020, 12:13 PM.

Details

Summary

Return values from async regions as !async.value<...>.

Diff Detail

Event Timeline

ezhulenev created this revision.Sep 29 2020, 12:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2020, 12:13 PM
ezhulenev requested review of this revision.Sep 29 2020, 12:13 PM
mehdi_amini added inline comments.Sep 29 2020, 1:08 PM
mlir/include/mlir/Dialect/Async/IR/AsyncBase.td
51
mlir/lib/Dialect/Async/IR/Async.cpp
74

Can you add a class doc (something simple like "Storage for async.value<T> type, the only member is the wrapped type.")

103

Please fix :)

106

This can fail, you need to check in the verifier. Also I rather be more strict here: we really want to map the immediate parent only:

114

Nit: remote trivial braces.

131

interleaveComma should do it here.

This can't be done in the assembly format by the way?

You're also missing attribute printing

ezhulenev updated this revision to Diff 295103.Sep 29 2020, 1:39 PM
ezhulenev marked 5 inline comments as done.

Update parse/print + fix linter warning

mlir/lib/Dialect/Async/IR/Async.cpp
106

Wouldn't this be checked by HasParent trait defined on .td file?

131

Changed to interleaveComma.

Assembly format does not seem to work for variadic results.

Added attrs parsing/printing.

ezhulenev updated this revision to Diff 295104.Sep 29 2020, 1:45 PM

Verify async.yield op parent

mlir/lib/Dialect/Async/IR/Async.cpp
106

Didn't find anything like that in the generated file, added the check to this function.

mehdi_amini accepted this revision.Sep 29 2020, 1:47 PM
mehdi_amini added inline comments.
mlir/lib/Dialect/Async/IR/Async.cpp
106

You're right: the HasParent trait will verify it (there isn't a specific verification in the TableGen generated file, it is dispatched to the trait by the simple fact of inheriting from it)

This revision is now accepted and ready to land.Sep 29 2020, 1:47 PM
ezhulenev updated this revision to Diff 295110.Sep 29 2020, 1:56 PM

Do not check async.yield parent op in verifier, it is already checked by HasParent trait.

csigg accepted this revision.Sep 30 2020, 11:09 AM

Looks good!

Just name bike shedding: I assume you considered both async.value and async.future. What's your reason to pick async.value?

What's your reason to pick async.value?

When I was writing this PR I didn't see the async.future proposal ;) Although I'm not a fan of async.future because both async and future have very similar semantics, and having them both in the same type looks weird to me. I'm fine changing it to future, if people have strong opinion about that. I'll prepare the next PR with async arguments, and then we can change the type if needed.

ezhulenev marked an inline comment as done.Sep 30 2020, 11:25 AM
This revision was automatically updated to reflect the committed changes.
csigg added a comment.Oct 1 2020, 1:52 AM

Although I'm not a fan of async.future because both async and future have very similar semantics...

I see where you are coming from. I look at !async.<type> as <type> in the async dialect, and that way future makes more sense than value.