This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Use memref memory space for storage class
ClosedPublic

Authored by antiagainst on Mar 16 2020, 11:27 AM.

Details

Summary

Previously in SPIRVTypeConverter, we always convert memref types
to StorageBuffer regardless of their memory spaces. This commit
fixes that to let the conversion to look into memory space
properly. For this purpose, a mapping between SPIR-V storage class
and memref memory space is introduced. The mapping is arbitary
decided at the moment and the hope is that we can leverage
string memory space later to be more clear.

Now spv.interface_var_abi cannot contain storage class unless it's
attached to a scalar value, where we need the storage class as side
channel information. Verifications and tests are properly adjusted.

Diff Detail

Event Timeline

antiagainst created this revision.Mar 16 2020, 11:27 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rriddle added inline comments.Mar 16 2020, 11:57 AM
mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
870–873

nit: ///

mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
41

nit: ///

mlir/lib/Dialect/SPIRV/TargetAndABI.cpp
15

Is this not included transitively already?

mravishankar requested changes to this revision.Mar 16 2020, 12:03 PM
mravishankar added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
885–891

Nit : Update the error message

mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
48

we should try to keep it close to what GPU dialect uses. There it is using a convention that NVVM adopts (https://docs.nvidia.com/cuda/nvvm-ir-spec/index.html#address-spaces).

I think this mapping would be most consistent (there is no direct mapping)

StorageBuffer : 0, 1
Workgroup : 3
PushConstant : 4
Private : 5.

Lets leave 2 out since it is reserved.

This revision now requires changes to proceed.Mar 16 2020, 12:03 PM
antiagainst marked 8 inline comments as done.

Address comments

antiagainst added inline comments.Mar 17 2020, 6:57 AM
mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
885–891

I plan to turn this into a proper SPIR-V dialect attribute too. It's becoming more annoying seeing the complicated internal structure of a dictionary attribute and we'll have more fields later.

mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
48

Adjusted. I guess the benefit of having similar mapping is mostly cognitive so that folks familiar with the NVVM mapping is not that surprised. But I'd avoid having non-1-1 mapping given it's not round trippable and also confusing. So for now,

Generic: 1
StorageBuffer: 0
Workgroup: 3
Uniform: 4
Private: 5
Function: 6
PushConstant: 7

We can swap Generic and StorageBuffer later after IREE side is fixed.

mlir/lib/Dialect/SPIRV/TargetAndABI.cpp
15

Yes. I think it was added by YouCompleteMe. Updated and tweaked some configuration for my vim plugins, now seeing this.

bondhugula requested changes to this revision.Mar 17 2020, 7:49 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/IR/Types.h
172

Typo: of this -> if this

174

of this -> if this

This revision now requires changes to proceed.Mar 17 2020, 7:49 AM
mravishankar accepted this revision.Mar 17 2020, 2:49 PM

Address comments

rriddle accepted this revision.Mar 18 2020, 1:26 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 18 2020, 5:24 PM
This revision was automatically updated to reflect the committed changes.