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

[DirectX] Update "dx.TypedBuffer" docs to include a "signed" bit #100695

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

bogner
Copy link
Contributor

@bogner bogner commented Jul 26, 2024

To lower these types to dxil we need to know whether ints are signed or not, but the LLVM type loses that. Add a bit to indicate it's so.

To lower these types to dxil we need to know whether ints are signed
or not, but the LLVM type loses that. Add a bit to indicate it's so.
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 26, 2024

@llvm/pr-subscribers-backend-directx

Author: Justin Bogner (bogner)

Changes

To lower these types to dxil we need to know whether ints are signed or not, but the LLVM type loses that. Add a bit to indicate it's so.


Full diff: https://github.com/llvm/llvm-project/pull/100695.diff

1 Files Affected:

  • (modified) llvm/docs/DirectX/DXILResources.rst (+12-9)
diff --git a/llvm/docs/DirectX/DXILResources.rst b/llvm/docs/DirectX/DXILResources.rst
index 5bbe902d9d2d7..8f7b11f7255c0 100644
--- a/llvm/docs/DirectX/DXILResources.rst
+++ b/llvm/docs/DirectX/DXILResources.rst
@@ -96,7 +96,7 @@ Buffers
 
 .. code-block:: llvm
 
-   target("dx.TypedBuffer", ElementType, IsWriteable, IsROV)
+   target("dx.TypedBuffer", ElementType, IsWriteable, IsROV, IsSigned)
    target("dx.RawBuffer", ElementType, IsWriteable, IsROV)
 
 We need two separate buffer types to account for the differences between the
@@ -106,9 +106,10 @@ used for DXIL's RawBuffers and StructuredBuffers. We call the latter
 "RawBuffer" to match the naming of the operations, but it can represent both
 the Raw and Structured variants.
 
-For TypedBuffer, the element type must be an integer or floating point type.
-For RawBuffer the type can be an integer, floating point, or struct type.
-HLSL's ByteAddressBuffer is represented by an `i8` element type.
+For TypedBuffer, the element type must be an scalar integer or floating point
+type, or a vector of at most 4 such types. For RawBuffer the type can be an
+integer, floating point, vecvtor, or struct type. HLSL's ByteAddressBuffer is
+represented as a RawBuffer with an `i8` element type.
 
 These types are generally used by BufferLoad and BufferStore operations, as
 well as atomics.
@@ -128,6 +129,8 @@ There are a few fields to describe variants of all of these types:
        writeable) and UAVs (writeable).
    * - IsROV
      - Whether the UAV is a rasterizer ordered view. Always ``0`` for SRVs.
+   * - IsSigned
+     - Whether the element type is signed ("dx.TypedBuffer" only)
 
 .. _bufferLoad: https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/DXIL.rst#bufferload
 .. _bufferStore: https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/DXIL.rst#bufferstore
@@ -197,23 +200,23 @@ Examples:
 .. code-block:: llvm
 
    ; RWBuffer<float4> Buf : register(u5, space3)
-   %buf = call target("dx.TypedBuffer", float, 1, 0)
+   %buf = call target("dx.TypedBuffer", <4 x float>, 1, 0, 0)
                @llvm.dx.handle.fromBinding.tdx.TypedBuffer_f32_1_0(
                    i32 3, i32 5, i32 1, i32 0, i1 false)
 
-   ; RWBuffer<uint> Buf : register(u7, space2)
-   %buf = call target("dx.TypedBuffer", i32, 1, 0)
+   ; RWBuffer<int> Buf : register(u7, space2)
+   %buf = call target("dx.TypedBuffer", i32, 1, 0, 1)
                @llvm.dx.handle.fromBinding.tdx.TypedBuffer_i32_1_0t(
                    i32 2, i32 7, i32 1, i32 0, i1 false)
 
    ; Buffer<uint4> Buf[24] : register(t3, space5)
-   %buf = call target("dx.TypedBuffer", i32, 0, 0)
+   %buf = call target("dx.TypedBuffer", <4 x i32>, 0, 0, 0)
                @llvm.dx.handle.fromBinding.tdx.TypedBuffer_i32_0_0t(
                    i32 2, i32 7, i32 24, i32 0, i1 false)
 
    ; struct S { float4 a; uint4 b; };
    ; StructuredBuffer<S> Buf : register(t2, space4)
-   %buf = call target("dx.RawBuffer", {<4 x f32>, <4 x i32>}, 0, 0)
+   %buf = call target("dx.RawBuffer", {<4 x float>, <4 x i32>}, 0, 0)
                @llvm.dx.handle.fromBinding.tdx.RawBuffer_sl_v4f32v4i32s_0_0t(
                    i32 4, i32 2, i32 1, i32 0, i1 false)
 

llvm/docs/DirectX/DXILResources.rst Outdated Show resolved Hide resolved
Comment on lines 132 to 133
* - IsSigned
- Whether the element type is signed ("dx.TypedBuffer" only)
Copy link
Contributor

Choose a reason for hiding this comment

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

What would it mean to have an unsigned float? I don't quite understand how this isn't inferable from the element type. Wonder if some more explanation in this doc might be warranted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a couple of sentences to make this clearer

; RWBuffer<uint> Buf : register(u7, space2)
%buf = call target("dx.TypedBuffer", i32, 1, 0)
; RWBuffer<int> Buf : register(u7, space2)
%buf = call target("dx.TypedBuffer", i32, 1, 0, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

You say in this doc that IsSigned is an extra parameter, but that param is only present for TypedBuffer. Yet, I see a new parameter here.
Maybe you meant to write somewhere that RWBuffers are represented as TypedBuffers? Should that be made explicit? (Like you specified with ByteAddressBuffers above).
Otherwise, shouldn't we specify that RWBuffers also get an extra parameter, IsSigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was fairly explicit already, but I've added some wording to be clearer about that.

@bogner bogner merged commit 135a1e9 into llvm:main Jul 29, 2024
8 checks passed
banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
…m#100695)

To lower these types to dxil we need to know whether ints are signed or
not, but the LLVM type loses that. Add a bit to indicate it's so.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants