This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add support for lowering scf.for scf/if with return values
ClosedPublic

Authored by ThomasRaoux on Jun 19 2020, 3:55 PM.

Details

Summary

This allow lowering to support scf.for and scf.if with results. As right now spv region operations don't have return value the results are demoted to Function memory. We create one allocation per result right before the region and store the yield values in it. Then we can load back the value from allocation to be able to use the result.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Jun 19 2020, 3:55 PM
Herald added a project: Restricted Project. · View Herald Transcript
antiagainst accepted this revision.Jun 24 2020, 4:16 PM

Cool, thanks!

mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
45 ↗(On Diff #272208)

This comment needs to be updated. It would be nice to give a high-level description of what we are doing here regarding yield handling.

58 ↗(On Diff #272208)

llvm::to_vector<4>(operands) will work here?

142 ↗(On Diff #272208)

s/IfOp/scf ops/

143 ↗(On Diff #272208)

There is no spv.if. :)

143 ↗(On Diff #272208)

s/doesn't have result/doesn't yield value/

144 ↗(On Diff #272208)

s/ForOp/scf op/

146 ↗(On Diff #272208)

s/Replace/replace/

160 ↗(On Diff #272208)

I think you can directly pass in spirv::StorageClass::Function here. We should have a build method for that.

mlir/test/Conversion/GPUToSPIRV/if.mlir
134

Let's add a TODO here to mention this is missing proper VariablePointer capability.

This revision is now accepted and ready to land.Jun 24 2020, 4:16 PM

I realized my previous patch breaks seriazliation/deserialization since those require merge block to only have merge op in it. I rebased my change on top of D82914 and use the scf context to keep track of the VariableOp created during lowering of the control flow region. This allows adding store when lowering yield operation instead and solve the problem with merge block.
Sorry for the mess this cause in the review process.

ThomasRaoux marked 9 inline comments as done.Jun 30 2020, 4:47 PM
ThomasRaoux added inline comments.
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
58 ↗(On Diff #272208)

This still needs to be merge with br.getBlockArguments() so I'm not sure how using to_vector helps?

160 ↗(On Diff #272208)

I couldn't get it to work. Maybe I'm missing something, I couldn't find any example in the code either.

rriddle added inline comments.Jul 1 2020, 1:38 PM
mlir/include/mlir/Conversion/SCFToSPIRV/SCFToSPIRV.h
35–36

Can you please comment on what this class is, and its intended purpose?

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
23

Please document what this is and what it is used for.

25

nit: Move these out of the mlir namespace and into the global namespace for the file.

42

nit: ///

84

nit: Please use /// for top-level comments.

89

Static methods should not be within namespaces. Please split this out of the anonymous namespace. Anonymous namespaces should not be used for functions.

94

nit: Please spell out auto here.

97

nit: Please spell out auto here.

106

nit: Please add a parameter comment here, /*...=*/nullptr

143

nit: Spell out auto here.

153–156

nit: Please cache the end iterator of for loops, and drop the int part of unsigned int.

for (unsigned i = 1, e = ...; i != e; ++i)
258

nit: Drop trivial braces, and cache the end iterator.

antiagainst accepted this revision.Jul 1 2020, 4:01 PM

Nice! Thanks for this! I just have a few more nits. Please address River's comments.

mlir/include/mlir/Conversion/SCFToSPIRV/SCFToSPIRV.h
35–36

+1

mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
58 ↗(On Diff #272208)

Oh sorry misread. :)

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
84

"Replaces SCF op outputs with SPIR-V variable loads."

87

s/srf/SCF/

252

If the region returns values, store ...

antiagainst added inline comments.Jul 1 2020, 4:06 PM
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
160 ↗(On Diff #272208)

I think we have a build methods that is:

static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type pointer, ::mlir::spirv::StorageClass storage_class, /*optional*/::mlir::Value initializer);
ThomasRaoux marked 14 inline comments as done.
This revision was automatically updated to reflect the committed changes.