SPIRV binary to MLIR hello world example

[hazem]

Makes sense. I will cleanup the code from the later changes I made, and get familiar with the contribution steps in the documenation and get back to you.

I am interested in addressing the layoutinfo one as well. I believe we need to do it if we want the matrix data type to be fully supported, and Array type too. I can see that the layoutinfo needs to be changed to a more generic data structure. If you have more information that can help here please let me know.

P.S: I tried to quote the text and reply inline but my post was hidden and I received a message that it was filtered for some reason that I don’t know yet.

Thanks

[hazem]

hello @antiagainst

Right now I am working on finalizing the Serializer for the Matrix type but I found an issue, in the spv.AccessChain, according to the specification, the type of the index should be an integer that will be treated as signed (SPIR-V Specification). And in the provided example in SPIRV Dialect you can see the index constant represented this way:

%0 = "spv.constant"() { value = 1: i32} : () -> i32
%1 = spv.Variable : !spv.ptr<!spv.struct<f32, !spv.array<4xf32>>, Function>
%2 = spv.AccessChain %1[%0] : !spv.ptr<!spv.struct<f32, !spv.array<4xf32>>, Function>

However, when I created a SPIRV binary for testing the Matrix data type, I get the following:

  %16 = spv.constant 2 : si32
  %17 = spv.AccessChain %1[%16] : !spv.ptr<!spv.matrix<3 x vector<3xf32>>, Function>

This si32 causes the following error in the AccessChain:

error: use of value '%16' expects different type than prior uses: 'i32' vs 'si32'

I traced this error to Parser.cpp → resolveSSAUse method. The method is expecting i32 type but it finds si32 so it raises this error.

To fix this issue, I modified the method parseAccessChainOp in SPIRVOps.cpp basically I replaced this line:

Type indicesType = parser.getBuilder().getIntegerType(32);

to be:

Type indicesType = parser.getBuilder().getIntegerType(32, true);

I no longer get the error above and I believe this is the cause of the error I posted earlier this month, and it should be compliant with the specification that the accesschain indices are signed (si32) not (i32). Please correct me if my analysis and fix here are not the expected behavior from the code.

[antiagainst]

Great findings, @hazem! Yes the spv.AccessChain’s index type parsing is indeed problematic and that’s a bug we need to fix. (Sorry again for all the rough edges you’ve been hitting thus far as you are trying to bring up one real shader from graphics pipeline! )

A principled fix actually involves more than just changing parseAccessChainOp’s indicesType. OpAccessChain in SPIR-V allows any scalar integer type to be the index. That means both signedness and bitwidth. So it should be able to accept si32, ui16, i64 or whatever. Changing the assumption from i32 to si32 is not fundamentally addressing the limitation there. (Note that the spec says the index is treated as a signed count, but it does not mean it must be a signed integer type. In SPIR-V how to interpret an integer is done per-instruction. This is make clear by SPIR-V Specification. Although due to historical reasons you might experience inconsistency on this front in various SPIR-V compilers/consumers/etc.). It means we need to additionally specify the type for the indices in spv.AccessChain’s assembly. So instead of something like

spv.AccessChain %ptr[%i0, %i1] : !spv.ptr<!spv.matrix<3 x vector<3xf32>>, Function>

We need something like

spv.AccessChain %ptr[%i0, %i1] : !spv.ptr<!spv.matrix<3 x vector<3xf32>>, Function>, si32, ui32

(Note the trailing types for the indices.)

With the above change, the type of the indices will come directly from the assembly when parsing so it will handle all cases.

Does the above make sense?

[hazem]

Hello @antiagainst, thanks for the reply and elaborate explanation.
Yes, it makes sense now. My fix allowed me to get a shader to work, but we cannot rely on it as a permanent fix. I had three patches ready to submit for review regarding basic matrix support. One of this was regarding the AccessChain issue.

Since I there are two things to address now: 1) struct of matrices/arrays, and 2) AccessChain, I will start working on (1) first as it is related to the matrix support, then I can move on to the AccessChain issue. My current fix should allow me to go through (1) without issues for now (hopefully).

I can submit the two patches related to: (1) matrix support, and (2) few decorators I added manually to get a shader to work, through Phabricator. Until then please let me know if you have resources or information related to the LayoutInfo in the struct and updating it to support complex types with richer decorators (e.g., Matrix, Array, etc.).

[Edit] a followup question, doesn’t “signed count” mean that uint16 is illegal in this case? it needn’t be an integer or a particular bitwidth but shouldn’t it be strictly signed?

Thanks

[antiagainst]

Hey @hazem, sorry for the late reply. I saw your patch ⚙ D80594 [MLIR][SPIRV] Adding new data type and decorators to SPIRV Dialect. Looks great! Thanks a lot for taking on this!

@hazem

resources or information related to the LayoutInfo in the struct and updating it to support complex types with richer decorators (e.g., Matrix, Array, etc.).

At the moment the struct type supports offsets and one spirv::Decoration on each field:

llvm/llvm-project/blob/0712eac76616a088f1f1183399049560e69c3506/mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h#L281-L283

I think what we want to do is to replace them with one struct containing everything:

enum class FieldLayoutInfoKind  {
  Offset,
  MatrixStride,
  RowMajor,
  ColumMajor,
  ...
};
struct FieldLayoutInfo {
  LayoutInfoKind kind: 4,
  uint32_t fieldIndex : 28,
  uint32_t payload
};

class StructType {
  ...
  static StructType get(ArrayRef<Type> memberTypes,
                                   ArrayRef<FieldLayoutInfo> = {});
  ...
}

The FieldLayoutInfoKind is used to differentiate what kind of layout info we have in the FieldLayoutInfo struct. Note that we cannot use fancy type inheritance/hierarchy here given that these information will be stored inside MLIR’s type uniquer and kept alive until MLIRContext deallocates. So you’ll need to explicitly call the copyInto function on allocator in StructTypeStorage. The existing implementation should give you examples on that. The fieldIndex specifies which field this layout info is attaching to. We should sort the ArrayRef according to it. payload is for additional parameters. We have that for Offset, MatrixStride. For some other decorations it can be unused.

This does mean that in printing, we need to decode this packed struct by ourselves by reading kind to decide how to interpret the payload and print. But the logic should be minimal there.

Regarding the assembly for the struct type, we should extend it to allow more decorations like

!spv.struct<f32 [0], !spv.matrix<...> [64, MatrixStride=M, RowMajor, ...], ...>

Hopefully the above clarifies. Let me know if anything is unclear. And again thanks for helping on this!!

[antiagainst]

@hazem

[Edit] a followup question, doesn’t “signed count” mean that uint16 is illegal in this case? it needn’t be an integer or a particular bitwidth but shouldn’t it be strictly signed?

Not really. Note that the spec says the index is treated as a signed count, but it does not mean it must be a signed integer type. In SPIR-V how to interpret an integer is done per-instruction. This is make clear by SPIR-V Specification. But this is a fuzzy enough point that you may see SPIR-V consumers not fully respect the spec.

[hazem]

hello @antiagainst, thank you for all the information. I will start working on this after concluding the matrix patch.

I have one more question, when I run define_enum.sh without any operands the documentation says that it just updates everything based on the SPIRV online specification.

However, doing this and then building the master branch returns this error:

SPIRV/SPIRVBase.td:738:16: error: Variable not defined: ‘SPV_KHR_ray_query’
Extension<[SPV_KHR_ray_query]>
^
Do you have any idea as of why this happens?

Thanks

[antiagainst]

Oh. That’s because SPV_KHR_ray_query is a new extension recently introduced. The list of extensions is manually added in SPIRVBase.td (as you can see it’s not in the autogen section). That’s because we don’t have them defined in SPIR-V’s machine-readable grammar; the list is at GitHub - KhronosGroup/SPIRV-Registry: SPIR-V specs. So you’ll need to manually define it in SPIRVBase.td. It should be easy; just follow other extensions there.

[hazem]

Got it. Thanks @antiagainst!

[hazem]

Hello @antiagainst, I started working on the Struct layoutinfo modification but I have a clarification question. I noticed that the current way layoutinfo is handled (mainly considering offset only) and the way you suggested separates the layoutinfo from the decorations. The same goes for everything else (parsing, printing …etc.). My understanding is that Offset, MatrixStride, ArrayStride … etc. are all decorators that partially specify the memory layout. Is there a specific reason that Struct has LayoutInfo and memberDecorationInfo being two different entities?

Can we for instance define something like that:

struct MemberDecoration{
  spirv::Decoration decoration;
  uint32_t fieldIndex;
  uint32_t payload;
};

and then use it to extract layout info if needed (e.g., spirv:Decoration::Offset is a layout) and then update this line to:

using MemberDecorationInfo = std::pair<uint32_t, MemberDecoration>;

Thanks!

[antiagainst]

@hazem

Is there a specific reason that Struct has LayoutInfo and memberDecorationInfo being two different entities?

I don’t think there is a specific reason for that. It’s mostly due to implementation order. IIRC, we initially only handle decorations on the struct itself and then we grow the ability to handle member offsets. Two data structures are used because it’s easier back then but that’s mostly implementation details. Please feel free to adjust as you see fit.

@hazem

struct MemberDecoration{
  spirv::Decoration decoration;
  uint32_t fieldIndex;
  uint32_t payload;
};

This might be fine but we are allowing more decorations than those valid ones here. The suggestion I have in the above only permits those valid ones.

@hazem

using MemberDecorationInfo = std::pair<uint32_t, MemberDecoration>;

We’ve already placed the member index in the struct so there is no need to duplicate it here as the first element in the pair?

[hazem]

I see. Thank you this is very informative. I agree that there is no need for the index in the pair, and that my suggested structure would allow any decoration, which is not correct. I will study whether there is a need for separation or not, I will keep you updated. Thanks!

[hazem]

Hello @antiagainst, I looked around in the code and SPIRV specification, and I have some questions before proceeding with this change. Based on my understanding, a structure can have any member type, is that correct?

If this is the case then we can allow any decoration type, and I noticed that the current implementation defines member decorations as an ArrayRef. The index in the Pair is used to identify which member each decoration is attached too, this is assuming that a member can have multiple decorators (e.g., MatrixStride and RowMajor), is that correct? if this is the case can we have something like this instead:

struct MemberDecorationInfo{
  uint32_t offset;
  uint32_t memberIndex;
  ArrayRef<MemberDecorationInfo> memberDecorators;
};

struct MemberDecoratorInfo {
  spirv::Decoration decoration;
  uint32_t payload;
};

This way we separate the offset decorator which is unique and required for struct members, and support multiple key-value/Unit decorators per member.

Please correct me if any of the above assumptions is incorrect or if there is a limitation that prevents us from using the proposed structures. Thanks!

[antiagainst]

@hazem

Based on my understanding, a structure can have any member type, is that correct?

Yes, and … no. Struct as a general type can certainly host different types inside. But keep in mind that structs in interface storage classes have more restrictions. By interface storage class here, it means the storage that crosses runtime-shader or shader-stage … interface. More about interfaces here. So a normal struct in Private/Function storage class might be able to contain bool types, a struct in Uniform/StorageBuffer cannot. This reflects the GPU architecture: interfaces are about crossing boundaries so you’ll need to be explicit to make sure everything fits together so it needs explicit layout and cannot have abstract types inside, etc.

We want decorations for the struct type for the interface cases. (For normal structs inside internal storage classes there is no need to have decorations for layout.) So we won’t see all the possible types and combination of things there. Instead, there is only a limited list of types we need to support. Other types may require extra hardware capabilities. The decorations come with them are also just the few listed. See 2.16.2. Validation Rules for Shader Capabilities for more details, especially the clause of “Composite objects in the StorageBuffer, PhysicalStorageBuffer, Uniform, and PushConstant Storage Classes must be explicitly laid out.” and following.

@hazem

This way we separate the offset decorator which is unique and required for struct members, and support multiple key-value/Unit decorators per member.

I think this is also fine. Actually this is just how we internally hold the data for the type so it might not matter that much. One comment with more level of nesting though is that we need to explicitly store this into the StructTypeStorage so “flat” layout might make things easier given that you don’t need to allocate first for the memberDecorators array, get the pointer, and then allocate again for the MemberDecoratorInfo array. But I can see this squeezes a few more bytes out. So your call here.

[hazem]

I see, thanks for the information

@antiagainst

think this is also fine. Actually this is just how we internally hold the data for the type so it might not matter that much. One comment with more level of nesting though is that we need to explicitly store this into the StructTypeStorage so “flat” layout might make things easier given that you don’t need to allocate first for the memberDecorators array, get the pointer, and then allocate again for the MemberDecoratorInfo array. But I can see this squeezes a few more bytes out. So your call here.

Would replacing the ArrayRef with a SmallVector make a difference here?

[antiagainst]

@hazem

Would replacing the ArrayRef with a SmallVector make a difference here?

Not really. We need to store the data into the StructTypeStorage so that it has a lifetime as the MLIRContext. Whether passing in data as ArrayRef or SmallVector here does not matter. We need to copy anyway.

[hazem]

Hello @antiagainst, I am almost done with the changes but I have a minor issue to discuss, given a test case such as this one:

// CHECK: func @struct_type_with_matrix(!spv.struct<!spv.matrix<3 x vector<3xf32>> [0, ColMajor, MatrixStride=16]>)
func @struct_type_with_matrix(!spv.struct<!spv.matrix<3 x vector<3xf32>> [0, ColMajor, MatrixStride=16]>)

It passes successfully without errors with the new modifications. However, if the order of the given decorations is changed (as shown below), and we already sort the decorations (lexicographical) before invoking the Get method, this leads to an error because we changed the expected return order of the decorations, my question is: is there an implicit assumption about the order of decorations? for the matrix case for instance, the SPIRV binary generator puts the ColMajor/RowMajor before MatrixStride, but is this something we can always rely on, or should we be able to handle any decorations order by keeping the output and input order intact?

// CHECK: func @struct_type_with_matrix(!spv.struct<!spv.matrix<3 x vector<3xf32>> [0, MatrixStride=16, ColMajor]>)
func @struct_type_with_matrix(!spv.struct<!spv.matrix<3 x vector<3xf32>> [0, MatrixStride=16, ColMajor]>)

[antiagainst]

Hey @hazem, it’s awesome to see that you almost get it ready!

We should allow flexibility when parsing the IR assembly. Essentially the IR assembly is there for ease of compiler development and debugging. (In compilers the transformation are done on in-memory IR data structures.) It’s annoying to always require specific ordering when writing the IR for testing purpose. (Also one can make an analogy here regarding the whitespace characters: if we always mandate a particular format of the input IR and say require tabs that would be really annoying.) But for IR assmbly printing, we should be fine to canonicalize it and print the alphabetically/etc. sorted form. (Again think about whitespace characters here.) There is no need to be exactly the same as the input IR assembly. That’s additional burden that does not buy us much. As for how we internally store these in memory, there are separate trade-offs to consider (memory layout, size, etc.) Hope this clarifies.

[hazem]

Hello @antiagainst, thanks for the reply, I submitted the patch but it is failing the build for some reason.

On a side note, I have this shader:

#version 450
precision highp float;
precision highp int;

void main(){
    int y = 5;
    if (y>1){
        y = y-1;
    }
    y = y+ 1000;
}

you can see that it contains very basic instructions, however when I try to deserialize a SPIRV binary of this shader, I get this error:

llvm-project/mlir/include/mlir/IR/Types.h:271: bool mlir::Type::isa() const [U = mlir::TupleType]: Assertion `impl && "isa<> used on a null type."' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.

If you remove any reference to any variable declared before the if statement, I can successfully deserialize the binary. It seems that the problem is that variables are defined in the op.selection region which is why the rest of the code cannot access it (variable Y in this case), do you have any hints as of why this happens? do you face the same issue in compute kernels?

[hazem]

@hazem

#version 450
precision highp float;
precision highp int;

void main(){
    int y = 5;
    if (y>1){
        y = y-1;
        y = y+ 1000;    
   } else {
        y = y+ 1000;
   }
}

It also works if changed to something as this. This means that inserting the header block in the selection region and replacing it with Branch Op is the reason why any reference to any variable in the header block after the if statement will crash the code. Is there a strict requirement of putting everything in the Selection Region?