Page MenuHomePhabricator

[mlir][GPUToSPIRV] Modify the lowering of gpu.block_dim to be consistent with Vulkan SPEC
ClosedPublic

Authored by mravishankar on Feb 7 2020, 5:26 PM.

Details

Reviewers
antiagainst

Diff Detail

Event Timeline

mravishankar created this revision.Feb 7 2020, 5:26 PM
antiagainst accepted this revision.Feb 8 2020, 6:06 AM

LGTM.

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

s/cause/because/

72

This explanation is a bit confusing to me. SPIR-V spec allows variables to have initializers; it's the Vulkan spec requires WorkgroupSize to be decorated onto constants. What about using the following:

... This is separate because in Vulkan workgroup size is exposed to shaders via a constant with WorkgroupSize decoration. So here we cannot generate a builtin variable; instead the infromation ...

256–257

Nit: getLaunchConfigIndex ? This actually does not check what the op is; it's just querying the launch configuration index.

436–437

Sort these alphabetically

mlir/test/Conversion/GPUToSPIRV/builtins.mlir
83–84

Let's add some notes here to explain where this magic number comes from.

This revision is now accepted and ready to land.Feb 8 2020, 6:06 AM

Please also fix the test.

mravishankar marked 5 inline comments as done.

Addressing comments

Addressing missed comment.

mravishankar marked an inline comment as done.Feb 8 2020, 6:23 PM
mravishankar added inline comments.
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
256–257

Are you saying it should. It just returns {} if it doesnt find the right attribute and attribute value.

herhut added inline comments.Feb 10 2020, 5:02 AM
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
261

Maybe llvm::StringSwitch?

mlir/test/Conversion/GPUToSPIRV/builtins.mlir
83

nit: from

96

I think we should either require that the gpu.launch has the same constant argument than what the SPIR-V lowering uses or update the description of the operation to state that the provided dimensions are just guidelines.

I do not understand where the need to pass the workgroup size comes from. Wouldn't it be sufficient to require that the arguments to the launch are constants? The pass that constructs the launch and earlier passes that do the tiling need to know this value anyway. So they could manifest them in IR as constant arguments to launch.

116

Maybe use a constant other than 1 here to differentiate from the default.