This is an archive of the discontinued LLVM Phabricator instance.

[mlir][GPUToSPIRV] Make spv.interface_var_abi attribute on arguments either be unspecified on all arguments to use default ABI, or be present on all arguments.
ClosedPublic

Authored by mravishankar on Apr 1 2020, 10:53 AM.

Diff Detail

Event Timeline

mravishankar created this revision.Apr 1 2020, 10:53 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Adding a fallback path to get a "default" ABI to not make this change
breaking.

mehdi_amini added inline comments.Apr 1 2020, 10:50 PM
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
347

The revision title says that it makes "spv.interface_var_abi attribute a required attribute", but this function seems to make it optional?

mravishankar marked an inline comment as done.Apr 1 2020, 11:07 PM
mravishankar added inline comments.
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
347

This is more a escape hatch and is legacy. When I initially set this up it was convenient, but this is very limited in its usage. I would actually prefer to not have this, and any realistic user of the SPIR-V lowering will not be able to use this on many devices. So I can change the comment to make this clearer.

antiagainst requested changes to this revision.Apr 2 2020, 8:07 AM
antiagainst added inline comments.
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
346

Generates ... Returns false if any argument already has ABI specified.

356

Let's put a comment here to explain why this if statement like:

We need to wrap scalar values in a struct to satisfy Vulkan's interface variable requirements. We choose StorageBuffer for holding the struct here.

Otherwise it might be a bit mysterious.

mlir/test/Conversion/GPUToSPIRV/if.mlir
19 ↗(On Diff #254311)

I guess this is changes made when this commit is truly breaking. But now we have a default behavior; we might want to revert these to simplify the tests.

mlir/test/Conversion/GPUToSPIRV/simple.mlir
32

Instead of checking the default behavior, test the explicit behavior? Also, please make sure the assignment is not sequential as the default behavior.

mlir/test/mlir-vulkan-runner/addf.mlir
13 ↗(On Diff #254311)

I think we can omit the changes here; the in-tree Vulkan runner follows the sequential scheme.

This revision now requires changes to proceed.Apr 2 2020, 8:07 AM
mravishankar marked an inline comment as done.

Addressing comments and changing description to better reflect the
change post modifications.

mravishankar retitled this revision from [mlir][GPUToSPIRV] !!Breaking change!! Make spv.interface_var_abi attribute a required attribute while lowering from gpu.func to spv.func. to [mlir][GPUToSPIRV] Make spv.interface_var_abi attribute on arguments either be unspecified on all arguments to use default ABI, or be present on all arguments..Apr 6 2020, 9:33 AM
antiagainst requested changes to this revision.Apr 20 2020, 11:25 AM
antiagainst added inline comments.
mlir/test/Conversion/GPUToSPIRV/if.mlir
40 ↗(On Diff #255381)

Let's revert changes to these tests. They should pass as-is. Applies to following unaffected tests.

mlir/test/Conversion/GPUToSPIRV/simple.mlir
38

I think we can just test the gpu.func itself without the wrapping gpu.module and the corresponding launch op? This way it's clearer regarding the intended use case of attaching these attributes: it gives us a way to use and control the CodeGen side as a stand-alone component. :)

This revision now requires changes to proceed.Apr 20 2020, 11:25 AM
mravishankar marked an inline comment as done.

Rebasing and addressing comments

mravishankar marked 6 inline comments as done.

Adressing comments

mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
346

Made it populates.

mlir/test/Conversion/GPUToSPIRV/if.mlir
40 ↗(On Diff #255381)

This is not really a change, the args are split, thats all. Will remove it anyway.

mlir/test/Conversion/GPUToSPIRV/simple.mlir
38

Apparently the gpu.module is needed (gpu.func must have this as parent). Removed the funcOp that invokes the kernel though.

Harbormaster completed remote builds in B56552: Diff 263625.
antiagainst accepted this revision.May 13 2020, 10:27 AM
antiagainst marked an inline comment as done.

It would be nice to have a shorter oneliner for the first paragraph of the commit message and put the details in the second paragraph.

mlir/test/Conversion/GPUToSPIRV/simple.mlir
38

Oh right. A recent change made gpu.func must appear in gpu.module

This revision is now accepted and ready to land.May 13 2020, 10:27 AM
This revision was automatically updated to reflect the committed changes.