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

Nabla intrinsic hlsl generator tool #2

Open
wants to merge 3 commits into
base: header_4_hlsl
Choose a base branch
from

Conversation

Hazardu
Copy link

@Hazardu Hazardu commented May 2, 2024

I added a readme to the tool to explain the two additional json files

A few of the things are hard coded such as the #include and namespace statements in the generated as its meant to be a replacement for files in nbl/builtin/hlsl/spirv_intrinsics,
Those might need to be manually adjusted in the generated HLSL file.

Comment on lines 2 to 54
"operand": [
{
"class": "Atomic",
"operandList": [
"*"
]
},
{
"class": "Conversion",
"operandList": [
"OpBitcast"
]
},
{
"class": "Bit",
"operandList": [
"OpBitFieldUExtract",
"OpBitFieldSExtract"
]
}
],
"builtins": [
{
"name": "HelperInvocation",
"type": "bool"
},
{
"name": "Position",
"type": "float32_t4",
"const": false
},
{
"name": "VertexIndex"
},
{
"name": "InstanceIndex"
},
{
"name": "NumWorkgroups"
},
{
"name": "WorkgroupId"
},
{
"name": "LocalInvocationId"
},
{
"name": "GlobalInvocationId"
},
{
"name": "LocalInvocationIndex"
}
]

Choose a reason for hiding this comment

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

we should export everything, and just add type and const fields to the grammar json in this repo

Choose a reason for hiding this comment

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

ok I get the point of this... somewhat (it limits what we export)

but I think type and const should be added directly to the json enties in spirv.core.grammar.json, basically the builtins section shouldn't exist, unless you want to just list (allow/block) the builtin names you wish to export

Copy link
Author

Choose a reason for hiding this comment

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

There are benefits to keeping type mapping in a separate file.
The biggest one i can think of is that by modifying the grammar file, any subseqent updates pulled from Khronos Group might result in having to solve merge conflicts.

What might also happen is the c++ code generator stops working once the JSON schema changes, but surely its just me being paranoid.

Choose a reason for hiding this comment

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

first one is moot

What might also happen is the c++ code generator stops working once the JSON schema changes, but surely its just me being paranoid.

test this, but we'll only ever add extra fields

Comment on lines 7 to 10
{
"kind": "IdResult",
"type": "T"
},

Choose a reason for hiding this comment

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

we should probably call type, cpp_type or hlsl_type instead

and apply it to IdResultType

Choose a reason for hiding this comment

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

hmm actually, you should have hlsl_type e.g. const T& -> this means constant, ext_reference

but also list generics separately (once for the whole function).

The reason is that sometimes you might end up with 4 template dependant parameters (if vk::SpirvType<> gets involved), and only 2 template args

Copy link
Author

Choose a reason for hiding this comment

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

I might add an optional field that allows the user to list generic typenames that dont fit in the standard "single alphabetic char, uppercase" , and then merge attributes, const and type into hlsl_type field.

Comment on lines 15 to 17
"attributes": [
"vk::ext_reference"
]
Copy link
Member

Choose a reason for hiding this comment

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

vk::ext_reference is actually the only attribute we can apply.

Btw we need to modify how we codegen, the reason is that [vk::ext_reference] P doesnt work in all cases, so we need BOTH

template<typename T>
[[vk::ext_instruction(spv::OpAtomicLoad)]]
T AtomicLoad([[vk::ext_reference]] T pointer, uint32_t memory, uint32_t semantics);

template<typename T, typename Ptr_T>
[[vk::ext_instruction(spv::OpAtomicLoad)]]
T AtomicLoad(Ptr_T pointer, uint32_t memory, uint32_t semantics);

imho instead of listing the attributes separately, we could deduce it from the type containing & (being a T& in this case)

Choose a reason for hiding this comment

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

obviously if you have a lot of & this turns into combinatorial explosion of overloads 2^parameters_with_overloads but thats the fault of microsoft/DirectXShaderCompiler#6578

Comment on lines +171 to +172
"type": "float32_t4",
"const": false

Choose a reason for hiding this comment

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

hmm I'd definitely take the const from the type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants