tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.74k stars 260 forks source link

[spirv] Add AccessChainOp operation. #52

Closed denis0x0D closed 5 years ago

denis0x0D commented 5 years ago

This patch implements a AccessChainOp. ebnf form:

access_chain_op ::= ssa-id `=` `spv.AccessChain` ssa-use
                        `[` ssa-use (',' ssa-use)* `]`
                        `:` pointer-type

Custom assembly exampes:

func @foo(%arg0 : index) -> () {
  %0 = spv.Variable : !spv.ptr<!spv.array<4x!spv.array<4xf32>>, Function>
  %1 = spv.AccessChain %0[%arg0, %arg0] : !spv.ptr<!spv.array<4x!spv.array<4xf32>>, Function>
  %2 = spv.Load "Function" %1 ["Volatile"] : f32
  return
}
func @access_chain_struct() -> () {
  %0 = spv.constant 1: index
  %1 = spv.Variable : !spv.ptr<!spv.struct<f32, !spv.array<4xf32>>, Function>
  %2 = spv.AccessChain %1[%0, %0] : !spv.ptr<!spv.struct<f32, !spv.array<4xf32>>, Function>
  return
}

I've some concerns about index type, regarding to the spec the index type must be an ConstantOp for the struct and an integer for other types, so, I've decided to take an IndexType from standard ops, but I'm not sure that's a right way to represent an index for spirv dialect. @antiagainst can you please take a look, I'll be appriciate for any feedback.

antiagainst commented 5 years ago

Good question. I think we want to use SPV_Integer here instead of index type for the following reasons:

Right now I'm reusing standard i<bitwidth> types for signed integer types in SPIR-V. SPIR-V can also have unsigned types but it has been a confusing point thus far because SPIR-V want the instruction to dictate whether to interpret the integer as singed or unsigned (so we have OpSDiv, OpUDiv, etc.), not the type. So I'm a bit hesitating to introduce a special dialect type for unsigned integers. But I guess it's needed to convey the information for deserialization.

denis0x0D commented 5 years ago

@antiagainst @River707 @MaheshRavishankar thanks for review! I've updated regarding to the comments. I still have some concerns about index type, in this patch, I'm using i32, but the spec says any integer type, so I've left some comments in the code with TODO.

denis0x0D commented 5 years ago

@antiagainst thanks a lot for review, I've updated it, can you please take a look. Thanks.

denis0x0D commented 5 years ago

@antiagainst @River707 thanks for review, updated to the latest comment.

denis0x0D commented 5 years ago

@antiagainst сan I help with other tasks that no one works on? Thanks.

antiagainst commented 5 years ago

@denis0x0D: that is awesome; thanks for your help! Let me find some other tasks. :)

antiagainst commented 5 years ago

Hey @denis0x0D,

We are working on basics to enable the conversion from other dialects to the SPIR-V dialect right now. Hopefully after a few days we will enable parallel development so that you can play with the conversion and possibly contribute there! :)

But at any time you are always welcome to add more op definitions given they are certainly needed anyway to have a full-fledged SPIR-V dialect! When doing that, just keep a few things in mind. Please ignore control flow related ops (section 3.32.17 in the spec) right now; we have separate plans for them. Please also make sure serialization and deserialization is covered with tests. Given that most of the stuff is auto-generated, there is no need for extensive tests if certain feature is already tested in a similar op (e.g., no need to test OpFAdd extensively given it's very similar to OpFMul which is already extensively tested). Other than that, feel free to pick whatever ops you'd like to implement or even flesh out some in a batch (like those binary arithmetic ops)! :)

denis0x0D commented 5 years ago

@antiagainst thanks a lot! I'll definitely implement some other ops! Thanks for detailed description!