Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NNNN] Target Extension Types for Inline SPIR-V and Decorated Types #105

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cassiebeckley
Copy link

This adds target extension types to represent SpirvType from HLSL 0011, Inline SPIR-V.

This adds target extension types to represent `SpirvType` from HLSL
0011, Inline SPIR-V.
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This looks good. We will start updating other proposals that will depend on this proposal.

To represent `vk::SpirvType` and `vk::SpirvOpaqueType` in LLVM IR, we will add
three new target extension types:

* `spirv.inline.Type`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you looked into how the size and alignment information can be encoded? How do you get TargetExtType::getLayoutType() to return the right value?

https://llvm.org/doxygen/classllvm_1_1TargetExtType.html#ae473a097292848f10f82f0961245e012

Are we okay with it returning void? It seems like the information comes from getTargetTypeInfo in Type.cpp. Will you be able to set this properly? Do we need optional integer arguments that have the size and alignment?

If we do have the size and alignment, what is the layout type?

Also, which properties should be set?

%ArrayTex2D = type target("spirv.inline.Type", %type_2d_image, %integral_constant_4, 28)
```

### Type decorations
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should mention that this is related to the decoration attribute in inline spir-v, which can only be applied to types and member variables. Talk about how that attribute will be translated into something of this type. I believe this has alternative solutions that we discussed. The might be worth mentioning in the alternative solutions seciton.

Lastly, you need to consider all versions of the decoration, decorateId and decorateString? Do we want multiple versions? Use the IngralConstant, Literal, and some other String class?

Sorry, I should have mentioned these variants when I asked you to include the decorations in this design.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I had not noticed int_spv_assign_decoration in https://github.com/llvm/llvm-project/blob/main/llvm/docs/SPIRVUsage.rst#target-intrinsics. We should explain why we are not using it, if we decide not to use it. If it does work for us, then that could be a better solution.

Comment on lines +98 to +99
of the SPIR-V Specification. This will create a unique SPIR-V type with the
specified `OpDecoration`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call out more explicitly this does not add a decoration to the id for type. In must create a new type with a new id.

Comment on lines +33 to +35
* `spirv.inline.Type`
* `spirv.inline.IntegralConstant`
* `spirv.inline.Literal`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we need inline in the name? I think it could be removed in all of the types.

`spirv.DecoratedType` target extension type.

```
target("spirv.DecoratedType", type, decoration)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some decorations have operands. This might need to be more general.

@llvm-beanz llvm-beanz self-requested a review December 5, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants