Page MenuHomePhabricator

Add 'gpu.terminator' operation.
ClosedPublic

Authored by herhut on Jan 29 2020, 5:01 AM.

Details

Summary

The 'gpu.terminator' operation is used as the terminator for the
regions of gpu.launch. This is to disambugaute them from the
return operation on 'gpu.func' functions.

This is a breaking change and users of the gpu dialect will need
to adapt their code when producting 'gpu.launch' operations.

Diff Detail

Event Timeline

herhut created this revision.Jan 29 2020, 5:01 AM

PTAL. I will do another breaking change that removes the use of region arguments in a followup.

Unit tests: pass. 62255 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Jan 29 2020, 5:23 AM

Thanks!

mlir/test/mlir-cuda-runner/shuffle.mlir
24

Nit: we may want to skip this terminator while printing, but I won't insist.

This revision is now accepted and ready to land.Jan 29 2020, 5:23 AM
herhut marked an inline comment as done.Jan 29 2020, 6:04 AM
herhut added inline comments.
mlir/test/mlir-cuda-runner/shuffle.mlir
24

I was wondering the same. The only reason I left it printed was that I don't like a basic block without terminator. Confused my mental parser. So I only make them implicit for single-block operations. We could drop it if there is only a single block? Or is that too incoherent?

ftynse added inline comments.Jan 29 2020, 6:10 AM
mlir/test/mlir-cuda-runner/shuffle.mlir
24

It sounds like too much code for sugaring the IR. Let's keep it visible all the time, having it consistent between single- and multiple-block regions is a good argument.

The 'gpu.terminator' operation is used as the terminator for the regions of gpu.launch. This is to disambugaute them from the return operation on 'gpu.func' functions.

(Nit: typo on disambugaute)

D71961 is going in the direction of generalizing std.return ; what is the guideline for adding a new op vs re-using std.return?

D71961 is going in the direction of generalizing std.return ; what is the guideline for adding a new op vs re-using std.return?

This diff introduces a terminator that is explicitly "not a return", due to the confusion that appeared in a previous change wrt its semantics. In particular, it is used inside gpu.launch potentially inside std.func, it is never expected to have operands. When the generalization of std.return, I would consider replacing gpu.return with std.return if compatible, but not gpu.terminator.

This revision was automatically updated to reflect the committed changes.

The 'gpu.terminator' operation is used as the terminator for the regions of gpu.launch. This is to disambugaute them from the return operation on 'gpu.func' functions.

(Nit: typo on disambugaute)

Did you fix this?

D71961 is going in the direction of generalizing std.return ; what is the guideline for adding a new op vs re-using std.return?

This diff introduces a terminator that is explicitly "not a return", due to the confusion that appeared in a previous change wrt its semantics. In particular, it is used inside gpu.launch potentially inside std.func, it is never expected to have operands. When the generalization of std.return, I would consider replacing gpu.return with std.return if compatible, but not gpu.terminator.

I don't think I agree with this. The discussion on generalizing std.return goes against this as far as I understand it.

Why was this committed while we're discussing here?

Why was this committed while we're discussing here?

I misunderstood the comment it seems. I though the idea was that gpu.return should be replaced by a generalized return. From reading the discussion on 'std.return' I assumed this was meant to become a generalized way of returning from a function-like structure.

This change prepares this by splitting terminators that are function like, especially in the sense that their return operands correspond to the parent operation's result value. For the return form a launchOp, this is not the case. Currently, launchOp does not yield any value but that will change soon when we start making things asynchronous. So it does not contradict the work on a generic return.