[SPIRVToLLVM] set an attribute or encode "location" in llvm.mlir.global

[Weiwei-2021]

I am investigating how to convert the following spv.GlobalVariable to llvm dialect

spv.GlobalVariable @o_out0 {location = 0 : i32} : !spv.ptr<i32, Output>

Currently, SPIRVToLLVM will just omit the location attribute and I will get:

llvm.mlir.global external @o_out0() : i32

I am thinking about setAttribute for the llvm::GlobalVariableOp, which can be done by simply adding code like:

    IntegerAttr location = op->getAttrOfType<IntegerAttr>("location");
    if(location) {
      newGlobalOp->setAttr("location", location); 
    }

And the result looks like:

llvm.mlir.global external @o_out0() {location = 0 : i32} : i32

But I notice that in SPIRVToLLVM.cpp, we have a function encodeBindAttribute. It encodes the “binding” and “descriptor_set” into the global variable name. Considering that “location” is also a layout qualifer as “binding” and “descriptor”. Do we prefer using the mechanism in encodeBindAttribut or setting “location” as an attribute of llvm.mlir.global is also fine?

Also based on our investigation, we will also encounter the same issue when converting image load store since image also attch many attributes to record its image type.

[antiagainst]

Hey @Weiwei-2021, thanks for your interest in the SPIR-V to LLVM conversion!

Before going into details, just wanted to remind you that it’s contributed by @george as a GSoC project. See the conversion manual and George’s summary here if you haven’t run into them.

George did a fantastic job and it’s a great contribution, but there are still quite some technical issues unaddressed (e.g., multithreaded cases, robust and flexible ABI support, etc.) to have it as something you can use for load-bearing workloads. What you are looking at here is effectively ABI, which isn’t fully fleshed yet. I’m curious, what would you like to achieve with this location support? Depending on your answer (just trying out things, or make it work with some your internal stack, etc.), the suggestion might be different. Could you elaborate on the intention a bit more?

Coming to the specific location support, a couple of points.

@Weiwei-2021

I am thinking about setAttribute for the llvm::GlobalVariableOp, which can be done by simply adding code like:

    IntegerAttr location = op->getAttrOfType<IntegerAttr>("location");
    if(location) {
      newGlobalOp->setAttr("location", location); 
    }

spv.GlobalVariable should actually have native support for it. Right now we only support descriptor set and binding and builtin. Even they are just specially treated in verification/conversion but not in ODS. These should all be in ODS as native attributes for better integration.

@Weiwei-2021

And the result looks like:

llvm.mlir.global external @o_out0() {location = 0 : i32} : i32

This is certainly doable. But I don’t think attaching the location to the resultant llvm.mlir.global is the final step. You need to do something with it later. Like if this is a driver compiler consuming SPIR-V input and generate architecture specific GPU ISA, you’ll need to pick up the attribute here and use it to link different graphics shader stages. Is that what you want? Or something else?

@Weiwei-2021

But I notice that in SPIRVToLLVM.cpp, we have a function encodeBindAttribute. It encodes the “binding” and “descriptor_set” into the global variable name. Considering that “location” is also a layout qualifer as “binding” and “descriptor”. Do we prefer using the mechanism in encodeBindAttribut or setting “location” as an attribute of llvm.mlir.global is also fine?

Please don’t read too much into the current descriptor set/binding treatment. They are there to serve the purpose of having a MVP to have a CPU-based mlir-spirv-cpu-runner. It should not be seen as the way of handling it. Effectively descriptor set/binding is a part of the ABI. The host and device exchange data through the contract there. They should match. In the mlir-spirv-cpu-runner (which is a MVP host) we decided to use the string encoding for that purpose. But I can image in production driver there can be other ways (like using attributes, etc.).

Location is similar to descriptor set/binding in a sense but it also has its own responsibly. It is also used for linking different shader stages. There are details in the Vulkan spec. I’d assume you want to do that after converting to LLVM (which is why you’d like to pass down the location). Is that correct?

@Weiwei-2021

Also based on our investigation, we will also encounter the same issue when converting image load store since image also attch many attributes to record its image type.

Not sure I fully follow here. Do you mean you are plumbing image support through the SPIR-V to LLVM conversion and seeing similar issues of attributes being dropped?

[Weiwei-2021]

@antiagainst

Before going into details, just wanted to remind you that it’s contributed by @george as a GSoC project. See the conversion manual and George’s summary here if you haven’t run into them.

Thank you for sharnig those! It provides a clear picture about the SPIRV->LLVM project.

@antiagainst

Like if this is a driver compiler consuming SPIR-V input and generate architecture specific GPU ISA, you’ll need to pick up the attribute here and use it to link different graphics shader stages. Is that what you want? Or something else?

This is exactly what I am trying to do, e.g., propogate the location and pick up it when coverting LLVM dialect to llvm IR.

@antiagainst

Please don’t read too much into the current descriptor set/binding treatment. They are there to serve the purpose of having a MVP to have a CPU-based mlir-spirv-cpu-runner .

Good to know about it. I checked the file and want to find a standard approach to do it. That is why I ask.

@antiagainst

Location is similar to descriptor set/binding in a sense but it also has its own responsibly. It is also used for linking different shader stages. There are details in the Vulkan spec . I’d assume you want to do that after converting to LLVM (which is why you’d like to pass down the location). Is that correct?

Yes, we need to know this location info at llvm IR level for further compilation. Also curious why the current mlir-spirv-cpu-runner doesn’t encounter this issue? The shaders it used don’t contain any location qualifer?

@antiagainst

Not sure I fully follow here. Do you mean you are plumbing image support through the SPIR-V to LLVM conversion and seeing similar issues of attributes being dropped?

I should not raise another topic at the last minute which is confusing.Sorry about it. I don’t have an example in my hand now. Let us disscuss it later when I successfully repeat this error.

In summary, using attribute to represent “location” is a feasible approach. Thank you for sharing those information. I will use add this attribute to llvm.mlir.globalvarible and submit a patch for it. Thanks again and have a good day.

[antiagainst]

@Weiwei-2021

Yes, we need to know this location info at llvm IR level for further compilation. Also curious why the current mlir-spirv-cpu-runner doesn’t encounter this issue? The shaders it used don’t contain any location qualifer?

mlir-spirv-cpu-runner is only handling the compute portion. So there is no location and graphics shader stage linking and such. But I’m glad that you are pushing further on the graphics support. Looking forward to what you come up with later!

@Weiwei-2021

In summary, using attribute to represent “location” is a feasible approach. Thank you for sharing those information. I will use add this attribute to llvm.mlir.globalvarible and submit a patch for it. Thanks again and have a good day.

I think you mean spv.GlobalVariable here? Yup, please go ahead. My only request is that please handle descriptor set/binding and builtin similarly given you are touching this part. Don’t’ want to be in a hybrid situation.

[Weiwei-2021]

@antiagainst

I think you mean spv.GlobalVariable here?

I made a typo. I mean “llvm.mlir.global”. To clarify, the spirv dialect has the locatino as an attribute already. Currently the llvm dialect resulting from a spv.GlobalVariable is:

llvm.mlir.global external @o_out0() : i32

What I want to do is to add location attribute for it. After this change, it will look like:

 llvm.mlir.global external @o_out0() {location = 0 : i32} : i32

@antiagainst

My only request is that please handle descriptor set/binding and builtin similarly given you are touching this part. Don’t’ want to be in a hybrid situation.

Based on this comment, you want to have some llvm.mlir.global looks like:

 llvm.mlir.global external @o_out0() {binding = 0 : i32, descriptor_set = 0 : i32} : i32

Do I understand correctly?

[antiagainst]

@Weiwei-2021

What I want to do is to add location attribute for it. After this change, it will look like:

 llvm.mlir.global external @o_out0() {location = 0 : i32} : i32

That should be fine. We can attach any attribute on any op in MLIR. Do you need to pass it down to llvm proper (that is, the traditional lllvm IR outside MLIR) though?

@Weiwei-2021

Based on this comment, you want to have some llvm.mlir.global looks like:

 llvm.mlir.global external @o_out0() {binding = 0 : i32, descriptor_set = 0 : i32} : i32

Do I understand correctly?

I actually meant to have first-class location/descriptor_set/descriptor_binding support in spv.GlobalVariable. That is, define them in ODS. Right now they are not. We only have the following:

  let arguments = (ins
    TypeAttr:$type,
    StrAttr:$sym_name,
    OptionalAttr<FlatSymbolRefAttr>:$initializer
  );

We should also have dedicated attributes for those ABI related attributes, like

let arguments = (ins
  ...
  OptionalAttr<I32Attr>:$location,
  ...
)

You can still lower and attach them as attributes when converting to the llvm dialect.

[Weiwei-2021]

@antiagainst

That should be fine. We can attach any attribute on any op in MLIR. Do you need to pass it down to llvm proper (that is, the traditional lllvm IR outside MLIR) though?

Yes, location is a necessary info. for LLVM IR we want to create. It decides where to emit the ouput values.

@antiagainst

I actually meant to have first-class location / descriptor_set / descriptor_binding support in spv.GlobalVariable . That is, define them in ODS. Right now they are not. We only have the following:

I upload a patch: ⚙ D110207 [mlir][SPIRVToLLVM] Propagate location attribute from spv.GlobalVariable to llvm.mlir.global .Let us discuss more details in the patch. Thanks and have a good night.