Page MenuHomePhabricator

[mlir] Use rewriting infrastructure in AsyncToLLVM
ClosedPublic

Authored by tpopp on Dec 4 2020, 1:15 PM.

Details

Summary

This is needed so a listener hears all changes during the dialect
conversion to allow correct rollbacks upon failure.

Diff Detail

Event Timeline

tpopp created this revision.Dec 4 2020, 1:15 PM
tpopp requested review of this revision.Dec 4 2020, 1:15 PM
tpopp updated this revision to Diff 309636.Dec 4 2020, 1:22 PM

Pass an OpBuilder instead of an optional Listener.

ezhulenev requested changes to this revision.Dec 4 2020, 1:52 PM
ezhulenev added inline comments.
mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp
389–390

This is a bit misleading after the change. It would be nice to explain what exactly is the op and how splitBlock and resume were constructed (also I'd rename splitBlock to suspended).

Something along this line: "Add a LLVM coroutine suspension point to the end of suspended block, to resume execution in resume block. These blocks are the result of splitting blah blah"

This revision now requires changes to proceed.Dec 4 2020, 1:52 PM
ezhulenev added inline comments.Dec 4 2020, 1:53 PM
mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp
395–412

This example also will be out of date.

Harbormaster completed remote builds in B81157: Diff 309636.
tpopp updated this revision to Diff 310034.Dec 7 2020, 3:10 PM

Update comments.

tpopp marked 2 inline comments as done.Dec 7 2020, 6:52 PM
ezhulenev accepted this revision.Dec 8 2020, 2:09 AM
This revision is now accepted and ready to land.Dec 8 2020, 2:09 AM
ftynse added inline comments.Dec 8 2020, 4:14 AM
mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp
518

Builder will not be aware of this operation.

tpopp marked an inline comment as done.Dec 8 2020, 7:29 AM
tpopp added inline comments.
mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp
518

That's okay because this function is not using the DialectConversion infrastructure. This function uses normal OpBuilders and is an irreversible transformation.

ftynse accepted this revision.Dec 8 2020, 8:26 AM
ftynse added inline comments.
mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp
518

Maybe add a note to the doc that indicates this. It could end up confusing somebody that some functions are rewriter-compatible and some others are not.

tpopp marked 2 inline comments as done.Dec 8 2020, 8:29 AM
This revision was landed with ongoing or failed builds.Dec 8 2020, 8:30 AM
This revision was automatically updated to reflect the committed changes.