This is an archive of the discontinued LLVM Phabricator instance.

Add `gpu.create_token` op.
AbandonedPublic

Authored by csigg on Oct 7 2020, 8:42 AM.

Details

Reviewers
herhut

Diff Detail

Event Timeline

csigg created this revision.Oct 7 2020, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2020, 8:42 AM
csigg requested review of this revision.Oct 7 2020, 8:42 AM
mehdi_amini added inline comments.Oct 7 2020, 10:54 AM
mlir/include/mlir/Dialect/GPU/GPUBase.td
60 ↗(On Diff #296694)

Is this interface more generic than the GPU dialect? Can you include GPU in the name to avoid confusion with the async dialect.

62 ↗(On Diff #296694)

Please extend the documentation.

83 ↗(On Diff #296694)

if constexpr is a c++17 feature, LLVM is using C++14.

92 ↗(On Diff #296694)

Nit: this is a non-trivial block of C++, can this go inside an implementation file?

mlir/include/mlir/Dialect/GPU/GPUOps.td
759

Is ForkOp intended here?

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
327 ↗(On Diff #296694)

This hunk seems unrelated to this revision, please split and commit ahead.

Actually seems like you generated a mixed diff that includes changes from https://reviews.llvm.org/D88972 ?

csigg added a comment.Oct 7 2020, 1:06 PM

Actually seems like you generated a mixed diff that includes changes from https://reviews.llvm.org/D88972 ?

This is a follow-up change. Is there a better way than creating a new revision and adding the parent revision in phabricator?

Phab works with "diff", somehow you create this revision as a diff against master? In general I do an interactive rebase, edit individually each commit and arc diff HEAD~

csigg updated this revision to Diff 296921.Oct 8 2020, 4:25 AM

Remove base revision, address comments.

csigg marked 4 inline comments as done.Oct 8 2020, 4:28 AM
csigg added inline comments.
mlir/include/mlir/Dialect/GPU/GPUBase.td
60 ↗(On Diff #296694)

Sorry, this was meant to be in the ::mlir::gpu namespace. I left the name unchanged.

mlir/include/mlir/Dialect/GPU/GPUOps.td
759

Oops. Thanks for catching that.

csigg added a comment.Oct 8 2020, 4:29 AM

Phab works with "diff", somehow you create this revision as a diff against master? In general I do an interactive rebase, edit individually each commit and arc diff HEAD~

Got it, thanks.

herhut accepted this revision.Oct 9 2020, 5:04 AM
This revision is now accepted and ready to land.Oct 9 2020, 5:04 AM
csigg abandoned this revision.Oct 13 2020, 8:55 AM