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

[Feature Request] [SPIR-V] [Proposal 0011] vk::addrof(v) to obtain an OpTypePointer to a variable and deprecate [vk::ext_reference] #6578

Closed
devshgraphicsprogramming opened this issue May 2, 2024 · 11 comments
Labels
enhancement Feature suggestion spirv Work related to SPIR-V

Comments

@devshgraphicsprogramming

Is your feature request related to a problem? Please describe.

When writing inline SPIR-V intrinsics in HLSL one needs to choose between making an input argument T, [vk::ext_reference] T or vk::SpirvType.

Its not well documented what ext_reference actually turns into but from my experience it seems to be and OpTypePointer to the actual argument invoked.

This makes it problematic to write an intrinsic signature thats supposed to work both with T& and arbitrary vk::SpirvType, for example OpAtomicIAdd .

If you write

template<typename T>
[[vk::ext_instruction]]
T atomicIAdd([[vk::ext_reference]] T ptr, ...memory semantics... , T value);

then it will work on RWStructuredBuffer variables, but won't work on user-defined BDA pointers (or any pointers).

But if you write

template<typename T, typename P>
[[vk::ext_instruction]]
T atomicIAdd(P ptr, ...memory semantics... , T value);

then it will only work on vk::SpirvType defined pointers.

Which wouldn't be too bad, except that you can't get a pointer/access-chain to any variable you like.

Describe the solution you'd like

A way to obtain an OpTypePointer for a given variable (basically a hidden &/addrof)

Also a way to obtain an OpType result id, because some instrinsics like OpDecorate or OpMemberDecorate need to be fed the type's %result_id as an argument, its unclear to me how to achieve that with the current mapping of.

[[vk::ext_instruction(%opcode%)]]
%result_type% intrinsicHLSLName(arguments...);

Describe alternatives you've considered

Just generating an overload, but that makes the compiler confused with ambiguity.

Additional context

Trying to generate all Types and Operations from SPIR-V Headers.

Also implement Prop 0010.

@devshgraphicsprogramming
Copy link
Author

devshgraphicsprogramming commented May 6, 2024

Here's what I tried in the meantime to return me a vk::SpirvType<OpTypePointer,StorageClass,T>

OpAccessChain with no Indices

OpBitcast with a [[vk::ext_reference]] input casting to identical type

  • fails to observe side effects on stores because of aliasing spirv-opt doesn't account for
  • fails to compile on loads because something something "isn't a Logical Pointer"

Conclusion

There's nothing in SPIR-V I can use, I need a dedicated addrof. However that addrof needs to return a certain type....

The bugs? What could be happening?

The answer is simple, it seems that DXC is not-deduplicating the Op.....Type declarations.

and in my simple case here https://godbolt.org/z/M3ra87ro8

I'm getting two result IDs that declare a pointer to a uint32_t with the Workgroup storage space.

I think this behaviour might be the underlying cause for:

It kinda seems like the type-cache needs to be unified for the SPIR-V codegen, and regular HLSL types need to know what their SpirvType or SpirvOpaqueType is.

Or alternatively some legalization pass to dedup types.

@llvm-beanz llvm-beanz moved this to For Google in HLSL Triage May 7, 2024
@sudonatalie
Copy link
Collaborator

Requests for new HLSL language features (including Vulkan-specific attributes) now fall under the scope of the microsoft/hlsl-specs repository. Please consider submitting a proposal there instead.

@github-project-automation github-project-automation bot moved this from For Google to Triaged in HLSL Triage May 9, 2024
@sudonatalie sudonatalie closed this as not planned Won't fix, can't repro, duplicate, stale May 9, 2024
@sudonatalie sudonatalie added spirv Work related to SPIR-V and removed needs-triage Awaiting triage labels May 9, 2024
@devshgraphicsprogramming
Copy link
Author

Requests for new HLSL language features (including Vulkan-specific attributes) now fall under the scope of the microsoft/hlsl-specs repository. Please consider submitting a proposal there instead.

What are you planning to do about the fact that the apirv types don't deduplicate with the built-inones which makes it impossible to truly use prop 0011?

@s-perron
Copy link
Collaborator

s-perron commented Jul 29, 2024

I've been thinking about this more. Does something like this work for you: https://godbolt.org/z/xdEMd4bTo.

Edit: updated link

@devshgraphicsprogramming
Copy link
Author

devshgraphicsprogramming commented Jul 29, 2024

I've been thinking about this more. Does something like this work for you: https://godbolt.org/z/xdEMd4bTo.

Edit: updated link

it seems to be on the right track, but I still can't construct my own access chains
https://godbolt.org/z/c3Mr64ev9

Edit 1: It also doesn't seem to work on composite structs https://godbolt.org/z/rahE3qvcG

@s-perron
Copy link
Collaborator

Edit 1: It also doesn't seem to work on composite structs https://godbolt.org/z/rahE3qvcG

That happens if you remove -Vd for the scalar case as well. It is a missed optimization. In the inline spir-v is working as intended. If you add -Vd, there is the following dead code that is not removed:

      %21 = OpVariable %_ptr_Function_spirvIntrinsicType Function
               OpStore %21 %val

@devshgraphicsprogramming
Copy link
Author

Its a shame I need to disable validation to make it work, but thats ok for now.

But I still can't construct my own access chains:
https://godbolt.org/z/7fjzcq3s3

@devshgraphicsprogramming
Copy link
Author

@s-perron your intrinsic had the wrong signature, AtomicAdd returns a variable of type T and not a pointer
https://godbolt.org/z/bnzx1bjvP

So this actually has some limited usage as a addrof.

P.S. Still can't construct my own access chains

@devshgraphicsprogramming
Copy link
Author

devshgraphicsprogramming commented Aug 19, 2024

@s-perron there's a problem with DXC's SPIR-V codegen, there's no leak analysis it seems and everything gets aggressively optimized with O1, O2 and O3.

I cannot take references (SPIR-V pointers) to static static const or even function scope variables because they never get declared in the SPIR-V (no OpVariable).

Example
https://godbolt.org/z/vvdvYM4Wv

Note that this holds even if I start adding more funny things like conditionals. As long as the compiler wants/can optimize away any expression involving a variable I want to take a reference of, it will, same with loads. Basically the side-effects of assuming all references are restrict.

Therefore a real addrof() is needed to "tag" that the variable cannot be optimized away with O1 and above.

Edit: I'm unsure how DXC works, does the SPIR-V backend always emit "as-if" for O0 and then gets SPIR-V Tools' Opt to do the O1,O2,O3 transforms, or does some of the pre-codegen change with different optimization options?

Edit 2: Does SPIR-V assume C++-like strict aliasing that variables pointed to by different pointer types don't alias? I can see that there's obviously a Validation error with the OpCopyObject copying between unrelated types.

@devshgraphicsprogramming
Copy link
Author

P.S. wouldn't OpCopyLogical (opcode 400) codegen valid SPIR-V w.r.t. the rule that OpCopyObject has to copy between identical types?

@devshgraphicsprogramming
Copy link
Author

devshgraphicsprogramming commented Oct 21, 2024

This is starting to be quite a blocker for trying to use Inline-SPIRV with anything image related
https://tinyurl.com/2zb4z5a3

Scratch that, this works because I don't need to put a Spir-V type inside a Spir-V type (simple image type, nothing derived thankfully)
https://tinyurl.com/29pa2ssh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature suggestion spirv Work related to SPIR-V
Projects
Archived in project
Development

No branches or pull requests

3 participants