Page MenuHomePhabricator

[mlir][SPIRVToLLVM] Propagate location attribute from spv.GlobalVariable to llvm.mlir.global
ClosedPublic

Authored by Weiwei-2021 on Sep 21 2021, 4:50 PM.

Details

Summary

This patch is mainly to propogate location attribute from spv.GlobalVariable to llvm.mlir.global. I keep the current parse and print approach of location since I feel that it is the autogen's direction.

It also contains three small changes. I cannot test spirv binary->spirv dialect -> llvm dialect without them. But it seems that we don't have test for spirv binary->mlir, so I did add tests.

  1. Remove the restriction on UniformConstant In SPIRVToLLVM.cpp;
  2. Remove the errorCheck on relaxedPrecision when deserializering SPIR-V in Deserializer.cpp
  3. In SPIRVOps.cpp, let ConstantOp takes signedInteger too.

co-authered: Alan Liu <alanliu.yf@gmail.com> and Xinyi Liu <xyliuhelen@gmail.com>

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Sep 21 2021, 4:50 PM
Weiwei-2021 requested review of this revision.Sep 21 2021, 4:50 PM
antiagainst requested changes to this revision.Sep 23 2021, 10:58 AM

we don't have test for spirv binary->mlir

We have roundtrip tests for it at https://github.com/llvm/llvm-project/tree/main/mlir/test/Target/SPIRV. It converts MLIR/SPIR-V dialect modules into spirv blobs and then convert back.

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVStructureOps.td
398

Nit: if (

401

Nit: can you move this builder to follow the one at L410 right now? It then will form the logical order of more generic -> more specific. :)

407

Nit: if (

mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
737

This can be a switch now given we have many cases. Also the comment needs to be updated.

749

Comments need to be updated.

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
265
mlir/test/Conversion/SPIRVToLLVM/memory-ops-to-llvm.mlir
70

Could you also add tests for using UniformConstant to prevent regression?

73

Hmm, I'm not sure you can have location on storage buffer variables. Should be Input or Output here?

This revision now requires changes to proceed.Sep 23 2021, 10:58 AM
Weiwei-2021 marked 7 inline comments as done.
Weiwei-2021 marked an inline comment as done.

Thank you for comments.

mlir/test/Conversion/SPIRVToLLVM/memory-ops-to-llvm.mlir
73

You are right. It is a lazy copy paste from the above test and I didn't realize the storage class is storage buffer.....Let me correct it to avoid confusion.

Weiwei-2021 added inline comments.Sep 29 2021, 10:23 PM
mlir/test/Target/SPIRV/decorations.mlir
56

I will add a new line here.

antiagainst accepted this revision.Oct 4 2021, 7:15 AM
This revision is now accepted and ready to land.Oct 4 2021, 7:15 AM