This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add correct handling of Kernel and Addresses capabilities
ClosedPublic

Authored by kdobros on Aug 4 2020, 5:25 AM.

Details

Summary

This change adds initial support needed to generate OpenCL compliant SPIRV.
If Kernel capability is declared then memory model becomes OpenCL.
If Addresses capability is declared then addressing model becomes Physical64.
Additionally for Kernel capability interface variable ABI attributes are not
generated as entry point function is expected to have normal arguments.

Diff Detail

Event Timeline

kdobros created this revision.Aug 4 2020, 5:25 AM
kdobros requested review of this revision.Aug 4 2020, 5:25 AM

THanks! This is very welcome addition. Just a few minor comments below.

I am wondering if the Addresses capability should be done here. In general we have assumed that addressing mode is 32-bit, i.e. indexType is lowered to i32 in the SPIRVTypeConverter. We should make it 64 bits if the capability exists. It should then use 64-bit indexing out of the box, but that is untested. So seems like there is some work to be done on that aspect. The rest of the change seems unrelated.

Broader question though. RIght now there is an mlir-vulkan-runner that takes the generated SPIR-V binary and runs it to make sure the generated binary is correct. This also means that it is validated. There is no direct way of validating SPIR-V binary in MLIR tree, but wondering if you have used such a validation tool on the generated SPIR-V binary with Kernel capabilities?

mlir/test/Conversion/GPUToSPIRV/test_opencl_spirv.mlir
30 ↗(On Diff #282871)

Nit: Please address missing newline at end of file.

mravishankar requested changes to this revision.Aug 4 2020, 10:19 AM
This revision now requires changes to proceed.Aug 4 2020, 10:19 AM
antiagainst requested changes to this revision.Aug 4 2020, 3:22 PM

Awesome, great to see that you are interested in bring up the OpenCL conversion path! :)

I am wondering if the Addresses capability should be done here.

+1. Overall I'd suggest to have a central place to host this target execution environment related information. For example, add a new ExecutionEnvironment.{h|cpp} or something to have utility functions returning the addressing/memory/execution/etc. model. This way it avoids scattering such information in all the patterns and it's a better separation of concerns I think.

In general we have assumed that addressing mode is 32-bit, i.e. indexType is lowered to i32 in the SPIRVTypeConverter. We should make it 64 bits if the capability exists. It should then use 64-bit indexing out of the box, but that is untested. So seems like there is some work to be done on that aspect. The rest of the change seems unrelated.

+1. Good point Mahesh. I think the type converter should have access to the target env, so it can change to emit a different bitwidth for index if that's desired. It shouldn't be too hard but I guess there are places we are assuming 32-bit as pointed by Mahesh and that needs to be fixed. It can happen as a next step though.

mlir/lib/Dialect/SPIRV/Transforms/LowerABIAttributesPass.cpp
245

This seems a bit reversed. I think we should look into avoiding generating the ABI attributes in the first place if they are not useful?

kdobros updated this revision to Diff 283195.Aug 5 2020, 4:54 AM
kdobros edited the summary of this revision. (Show Details)

Separated functions to query execution environment related information
into ExecutionEnvironment{.h,.cpp}.
Removed generation of spv.interface_var_abi if it's not needed.
Fixed missing newline.

@mravishankar @antiagainst
Thank you for the reviews, I have made some changes addressing your comments.

Broader question though. RIght now there is an mlir-vulkan-runner that takes the generated SPIR-V binary and runs it to make sure the generated binary is correct. This also means that it is validated. There is no direct way of validating SPIR-V binary in MLIR tree, but wondering if you have used such a validation tool on the generated SPIR-V binary with Kernel capabilities?

This is a part of upstreaming PlaidML required changes from https://github.com/plaidml/plaidml/pull/1237. I am currently working on adding OpenCL+SPIRV path there and I have managed to get full pipeline, including kernel compilation, working.
There are still some issues with extended set of instructions that I'm planning to look at later.
Do you think it may be useful to consider adding similar mlir-opencl-runner in the future?

I am wondering if the Addresses capability should be done here. In general we have assumed that addressing mode is 32-bit, i.e. indexType is lowered to i32 in the SPIRVTypeConverter. We should make it 64 bits if the capability exists. It should then use 64-bit indexing out of the box, but that is untested. So seems like there is some work to be done on that aspect. The rest of the change seems unrelated.

I was looking at both Addresses and Kernel capabilities together, because they are both required to make the binary compile. Do you think I should split Addresses into separate change?

+1. Good point Mahesh. I think the type converter should have access to the target env, so it can change to emit a different bitwidth for index if that's desired. It shouldn't be too hard but I guess there are places we are assuming 32-bit as pointed by Mahesh and that needs to be fixed. It can happen as a next step though.

Agree, but if it's not a problem, I'd like to do this in separate change. It's currently not required to compile generated SPIR-V.
There is also the issue of selecting between Physical32 vs Physical64 and I have no idea how to select between those. Seems to me that spv.target_env may need some additional field, but I was little worried about breaking compatibility with other projects.

kdobros marked 2 inline comments as done.Aug 5 2020, 5:25 AM
mravishankar requested changes to this revision.Aug 5 2020, 9:12 AM

@mravishankar @antiagainst
Thank you for the reviews, I have made some changes addressing your comments.

Broader question though. RIght now there is an mlir-vulkan-runner that takes the generated SPIR-V binary and runs it to make sure the generated binary is correct. This also means that it is validated. There is no direct way of validating SPIR-V binary in MLIR tree, but wondering if you have used such a validation tool on the generated SPIR-V binary with Kernel capabilities?

This is a part of upstreaming PlaidML required changes from https://github.com/plaidml/plaidml/pull/1237. I am currently working on adding OpenCL+SPIRV path there and I have managed to get full pipeline, including kernel compilation, working.
There are still some issues with extended set of instructions that I'm planning to look at later.
Do you think it may be useful to consider adding similar mlir-opencl-runner in the future?

I think that would be very useful and a good way to make sure as we add OpenCL features we dont regress on Vulkan support. We heavily use Vulkan within IREE, so if something breaks they will show up there (cause of more extensive testing there). There are some mlir-vulkan-runner tests that will gives basic functionality testing. But having an OpenCL runner will provide the same stability for Compute capabilities, i.e. make sure that the OpenCL support doesnt regress with changes that are meant for Vulkan. Since there is no "in-tree" verifier, having a runner would be the best way to hook that up.

I am wondering if the Addresses capability should be done here. In general we have assumed that addressing mode is 32-bit, i.e. indexType is lowered to i32 in the SPIRVTypeConverter. We should make it 64 bits if the capability exists. It should then use 64-bit indexing out of the box, but that is untested. So seems like there is some work to be done on that aspect. The rest of the change seems unrelated.

I was looking at both Addresses and Kernel capabilities together, because they are both required to make the binary compile. Do you think I should split Addresses into separate change?

I think now the change looks better to me. So I just have a few more comments, and it should be fine to land.

+1. Good point Mahesh. I think the type converter should have access to the target env, so it can change to emit a different bitwidth for index if that's desired. It shouldn't be too hard but I guess there are places we are assuming 32-bit as pointed by Mahesh and that needs to be fixed. It can happen as a next step though.

Agree, but if it's not a problem, I'd like to do this in separate change. It's currently not required to compile generated SPIR-V.

There is also the issue of selecting between Physical32 vs Physical64 and I have no idea how to select between those. Seems to me that spv.target_env may need some additional field, but I was little worried about breaking compatibility with other projects.
spv.target_env definitely needs more attributes. With the mlir-vulkan-runner we get some basic level of testing. Completely fine with looking into this in a separate change.

mlir/include/mlir/Dialect/SPIRV/ExecutionEnvironment.h
22 ↗(On Diff #283195)

Mostly nit: If you just include SPIRVTypes.h you will get the enum definitions. THis is fine as well, so do not have strong opinion on this.

31 ↗(On Diff #283195)

I think we just use Optional<...> in MLIR AFAIK.

38 ↗(On Diff #283195)

I think most of these functions can go into TargetAndABI.h ?

mlir/lib/Dialect/SPIRV/ExecutionEnvironment.cpp
20 ↗(On Diff #283195)

I think most of these can go into TargetAndABI.cpp?

23 ↗(On Diff #283195)

Maybe add a TODO here to say that the Physical64 is being hard-wired here, but it should really come from the TargetEnvAttr itself.

This revision now requires changes to proceed.Aug 5 2020, 9:12 AM
kdobros added inline comments.Aug 6 2020, 7:55 AM
mlir/include/mlir/Dialect/SPIRV/ExecutionEnvironment.h
31 ↗(On Diff #283195)

I'm not sure about this, FailureOr<...> is defined inside MLIR, so if it's not used shouldn't it be removed?
I personally like the implication that if value is not present then something is wrong and function failed, and that's exactly the case here.
Just to be clear, I'm fine with changing this to Optional, but to me this seems a little confusing.

38 ↗(On Diff #283195)

Sure, looks like a fitting place to host these functions.
Just to clarify, is it okay to move all of them there?

mravishankar added inline comments.Aug 6 2020, 9:27 AM
mlir/include/mlir/Dialect/SPIRV/ExecutionEnvironment.h
31 ↗(On Diff #283195)

I got confused with something else (which I though was from core LLVM). I am not totally opposed to it, but its not used at all in the SPIR-V dialect. I think Optional is more consistent.

38 ↗(On Diff #283195)

I think it is OK to move it there.

kdobros updated this revision to Diff 283678.Aug 6 2020, 11:19 AM

Addressed review comments.
Minor style fixes - added explicit types if they don't hurt readability.

rriddle added inline comments.Aug 6 2020, 11:35 AM
mlir/include/mlir/Dialect/SPIRV/ExecutionEnvironment.h
31 ↗(On Diff #283195)

FailureOr is more recent, and should be used when it is more readable than Optional. This is a similar situation to why LogicalResult is preferred over bool.

mravishankar accepted this revision.Aug 6 2020, 12:18 PM

THanks! Looks good to me!

flaub added a subscriber: flaub.Aug 6 2020, 2:14 PM
antiagainst accepted this revision.Aug 6 2020, 2:28 PM

Cool, thanks for the contribution!

mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
67

... whether the given SPIR-V target ..

mlir/test/Conversion/GPUToSPIRV/test_opencl_spirv.mlir
1 ↗(On Diff #283678)

This should be named as model-structure-opencl.mlir or something to follow the convention.

This revision is now accepted and ready to land.Aug 6 2020, 2:28 PM
mravishankar added inline comments.Aug 6 2020, 3:01 PM
mlir/test/Conversion/GPUToSPIRV/test_opencl_spirv.mlir
11 ↗(On Diff #283678)

Forgot to mention this, but could you refactor the CHECK* lines a bit. It is more readable IMO if you group all the CHECK statements together and insert spaces to

  • align the ':' after CHECK* Statement
  • indent the checked code appropriately.

So

// CHECK-LABEL: func @foo
//  CHECK-SAME:  %[[ARG0:.*]]
//       CHECK:  %[[T0:.*]] =
kdobros updated this revision to Diff 283764.Aug 6 2020, 4:38 PM

Addressed comments.
Refactored not needed CHECKs in .mlir tests.
Brought back FailureOr, as per comment it is preferred over Optional.

@mravishankar @antiagainst
I think I addressed all the comments.
If you think the change is still good as is, could you commit on my behalf? I don't have commit access.
Name and email: Konrad Dobros <konrad.dobros@intel.com>

kdobros marked an inline comment as done.Aug 7 2020, 3:49 AM

Will commit this. THanks!