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

[0011] Resource element type validation #69

Merged
merged 21 commits into from
Oct 31, 2024
Merged
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7b0a53b
add intangible example, move proposal over
bob80905 Sep 13, 2024
cf382f7
include non-rawbuffer case, give examples
bob80905 Sep 16, 2024
2955655
rename, address greg
bob80905 Sep 17, 2024
55b8156
address Damyan, add some bullet lists
bob80905 Sep 18, 2024
38a0efe
use TypedBuffer instead of non-rawbuffer
bob80905 Sep 18, 2024
aeedba4
add info about textures, remove 32bit limit, dont code format rawbuffer
bob80905 Sep 19, 2024
2cd9493
introduce spir-v rules, discuss implementation of custom builtin type…
bob80905 Sep 20, 2024
46a67ae
fix typo
bob80905 Sep 23, 2024
5b9869e
address Chris and Damyan
bob80905 Sep 24, 2024
563aa4a
define is_spirv_target
bob80905 Sep 24, 2024
9f8ba5a
clarify type_trait implementation location, remove expected diagnostics
bob80905 Sep 25, 2024
2fb070c
simplify proposed solution, add eighthalves example, make type_traits…
bob80905 Sep 25, 2024
b7497e4
simplify by using __builtin_hlsl_is_line_vector_layout_compatible
bob80905 Sep 25, 2024
a2f38c1
incorporate design meeting feedback, remove is_complete_type, remove …
bob80905 Oct 3, 2024
fe417de
address Damyan
bob80905 Oct 4, 2024
6b19731
final touch of formatting
bob80905 Oct 4, 2024
5c0096c
small edits'
bob80905 Oct 21, 2024
739fe7d
insert ennum / bool constraint into builtin
bob80905 Oct 22, 2024
dd1c1bd
remove RET, remove mention of raw buffers, rename builtin
bob80905 Oct 30, 2024
f4a44ae
add back mention of raw buffers, remove references to line vector
bob80905 Oct 30, 2024
036cf48
rename builtin / concept, and rename filename
bob80905 Oct 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
223 changes: 223 additions & 0 deletions proposals/0010-resource-element-type-validation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
* Proposal: [0010](0010-resource-element-type-validation.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think we already have a proposal 10, so make sure to update the number before merging.

* Author(s): [Joshua Batista](https://github.com/bob80905)
* Sponsor: Joshua Batista
* Status: **Under Consideration**
* Impacted Project(s): (LLVM)
* Issues: [#75676](https://github.com/llvm/llvm-project/issues/75676)

## Introduction
Resources are often used in HLSL, with various resource element types (RETs).
hekota marked this conversation as resolved.
Show resolved Hide resolved

For example:
```
RWBuffer<float> rwbuf: register(u0);
```
In this code, the RET is `float`, and the resource type is `RWBuffer`.
All resources can be placed in two categories, raw buffers and typed buffers.
hekota marked this conversation as resolved.
Show resolved Hide resolved
Below is a description of all resources and their corresponding categories
* raw buffers
* [Append|Consume|RW|RasterizerOrdered]StructuredBuffer
* [RW]ByteAddressBuffer
* typed buffers
* [RW|RasterizerOrdered]Buffer
* [Feedback]Texture*

There is a distinct set of rules that define valid RETs for raw buffer resources,
and a separate set of rules that define valid RETs for typed buffer resources.

RETs for typed buffer resources:
* Are not intangible (e.g., isn't a resource type)
* Must be vectors or scalars of arithmetic types, not bools nor enums nor arrays
hekota marked this conversation as resolved.
Show resolved Hide resolved
* The type should be line-vector layout compatible (homogenous, at most 4 elements, and at most 128 bits in size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have always had a problem with this terminology.

  • It's inventing new terminology not connected to any prior terminology used for the concept to which it applies: "line-vector"? It's trying to sound more general than it actually is.
  • If you were to think about what it could/should be referring to, you could justifiably think it's about what fits into one constant buffer row (line?) in the legacy (current) constant buffer layout. But that would be a mistake, since the rules are different. (for example, you can put 8 native 16-bit values or a struct containing non-homogenous types in one row)
  • It implies that this type is used in the (memory) layout of the resource itself. It's not: the element stored and the layout is dependent on runtime factors: the DXGI resource type and device-dependent factors. The element type in HLSL for a typed resource is just how you want to interpret the values loaded from that up-to-4-component typed resource element.
  • The description sounds like it would support non-homogenous types (a struct with several different types that fit should work, as it would for constant buffers)
  • This 128-bit restriction sounds equivalent, but hides the true meaning: each 64-bit component in HLSL is translated to 2-32-bit components for the resource element type, with a bitcast for translation. This has implications for the actual element type used, compatible DXGI type mapping, and allowed operations. This has a limit of 2 64-bit components because those 2 components expand into a 4 component element type, not because it's 128 bits in size.
  • We could probably create specializations that handle 64-bit type translation directly to/from the translated element type in our HLSL header, rather than adding special-case code to the compiler. In this scenario, the real element type for the resource does not use a 64-bit type as the component type, but uses the translated element type with 32-bit uint components instead.

I would rather a more specific term be used for the more specific typed resource element constraint.

How about "typed-element compatible" and __builtin_hlsl_typed_element_compatible?

  • A "typed-element" flows from existing terminology: typed resources are textures and buffers with a DXGI type applying to the resource view descriptor at runtime. These support translation upon load/store to the HLSL element type used in a shader (depending on the types at either end and the operation being performed).
  • The element must be a scalar or vector of an integer or floating point type with a maximum of 4 components. All loads and stores are performed at element granularity, which is a 4 component vector (or struct) in the shader IR, whether or not all those components are read/defined.
  • If a 64-bit component type is used in HLSL, each 64-bit value maps to 2-32-bit uint components which will be reinterpreted together as the 64-bit value in HLSL. This has implications for supported operations, and the maximum number of 64-bit components allowed. double2 or [u]int64_t2 becomes uint4, which is the maximum number of components, and because the real component used is integer, it triggers those operation restrictions as well.

These above rules and description would apply to this "typed-element compatible"/__builtin_hlsl_typed_element_compatible constraint.

The short description could be:

  • slightly modified: "at most 4 homogenous scalars, and at most 128 bits in total size" (avoiding using "element" for scalar/component since the whole thing is the element type)
  • or "at most 4 homogenous scalars after translation of 64-bit scalars into pairs of uint32_t scalars". This makes the implied translation clear right in the short constraint description.
  • or subsume the scalar or vector and component type rule:
    "scalar or vector of a floating-point or integer type, with a maximum of 4 components after translating 64-bit components into pairs of uint32_t components"

There some additional constraints on the element type relating to the way the resource is used (methods called), but these would need to be enforced separately on the method calls:

  • If comparison sample (SampleCmp*) methods are used, the element type in HLSL must be a scalar floating point type of at most 32-bits.
  • If other Sample methods are used, integer components are only allowed in the element type when the shader model is 6.7 or greater. This is triggered by the use of double components because they break down into 2 uint32_t components.
    There are additional runtime restrictions for legal type combinations between the shader and the DXGI type, but these cannot be enforced by the compiler when they depend on runtime state. For instance: typed UAVs using a unorm or snorm DXGI type need to have a matching unorm float or snorm float component type in the HLSL resource element declaration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The naming used here is intended to be consistent with microsoft/hlsl-specs#321. Please comment on that PR if you think we need to adjust the naming there.


RETs for raw buffer variants are much less constrained:
* Are not intangible (e.g., isn't a resource type)
* All constituent types must be arithmetic types or bools or enums
tex3d marked this conversation as resolved.
Show resolved Hide resolved

Resource types are never allowed as RETs (i.e., `RWBuffer<int>` as an RET).
If someone writes `RWBuffer<MyCustomType>` and MyCustomType is not a
valid RET, there should be infrastructure to reject this RET and emit a message
explaining why it was rejected as an RET.

## Motivation
Currently, there is an allow list of valid RETs. It must be modified with respect
to this spec. Anything that is not a valid RET will be rejected. The allow list isn't
broad enough, because there is no case where the RET is user-defined for a raw buffer.
Ideally, a user should be able to determine how any user-defined structure is invalid
as an RET. Some system should be in place to more completely enforce the rules for
damyanp marked this conversation as resolved.
Show resolved Hide resolved
valid and invalid RETs, as well as provide useful information on why they are invalid.

For example, when targeting DXIL IR, `RWBuffer<double4> b : register(u4);` will emit
an error in DXC, but will not in clang-dxc, despite the fact that `double4` is an
invalid RET for typed buffers.

## Proposed solution

The proposed solution is to modify the declaration of each resource declared in
`clang\lib\Sema\HLSLExternalSemaSource.cpp` and insert into each representative
AST node a concept. The AST node will be created as if the C++20 `concept` keyword
was parsed and applied to the declaration. The concept will be used to validate the
given RET, and will emit errors when the given RET is invalid. Although concepts are
not currently supported in HLSL, we expect support to be added at some point in the
future. Meanwhile, because LLVM does support concepts, we can make use of
them when constructing the AST in Sema.

A new built-in, `__builtin_hlsl_is_line_vector_layout_compatible`, will be
added in order to express the extra typed buffer constraint. This builtin
will be added to each AST node that requires that constraint. The builtin is
described below. Standard clang diagnostics for unsatisfied constraints will be
used to report any invalid element types. Concepts required will differ depending
on whether the resource is a typed buffer or a raw buffer. Until concepts are
formally supported by HLSL, the concepts and constraints will be expressed
directly in the AST via the HLSL external sema source.

## Detailed design

In `clang\lib\Sema\HLSLExternalSemaSource.cpp`, `RWBuffer` is defined, along with
`RasterizerOrderedBuffer` and `StructuredBuffer`. It is at this point that the
concept would be incorporated into these resource declarations. A concept representing
the relevant constraints will be applied to each resource declaration. If a concept
is not true for the given RET, a corresponding error message will be emitted.

The list of builtins to be used as type traits that will be available for
concept definition are described below:
| type trait | Description|
|-|-|
| `__is_intangible` | An RET should be an arithmetic type, bool, enum, or a vector or matrix or UDT containing such types. This is equivalent to validating that the RET is not intangible. This will error when given an incomplete type. |
hekota marked this conversation as resolved.
Show resolved Hide resolved
| `__builtin_hlsl_is_line_vector_layout_compatible` | A typed buffer RET should never have two different subelement types. Line vector layout compatible also requires at most 4 elements, and a total size of at most 16 bytes. The builtin will also disallow the RET if any of its constituent types are enums or bools. |

For raw buffers, only `!__is_intangible` needs to be true.
For typed buffers, `__builtin_hlsl_is_line_vector_layout_compatible`
also needs to be true. This builtin will be used to ensure homogeneity.
It will use `BuildFlattenedTypeList` to retrieve a small vector of the subelement types.
From this subvector, the first element will be compared to all elements in the vector,
and any mismatches will return false. Typed buffer RETs will
also need to have at most 4 subelements, and the total size in bytes cannot exceed 16,
which will also be verified by `__builtin_hlsl_is_line_vector_layout_compatible`.
Finally, the builtin will validate that there are no bools or enums present in any
component of the type.

### Examples of RET validation results:
```
struct oneInt {
int i;
};

struct twoInt {
int aa;
int ab;
};

struct threeInts {
oneInt o;
twoInt t;
};

struct oneFloat {
float f;
};
struct notComplete;
struct depthDiff {
int i;
oneInt o;
oneFloat f;
};

struct notHomogenous{
int i;
float f;
};

struct EightElements {
twoInt x[2];
twoInt y[2];
};

struct EightHalves {
half x[8];
};

struct intVec {
int2 i;
};

struct oneIntWithVec {
int i;
oneInt i2;
int2 i3;
};

struct weirdStruct {
int i;
intVec iv;
};

RWBuffer<double2> r0; // valid - RET fits in 4 32-bit quantities
RWBuffer<int> r1; // valid
RWBuffer<float> r2; // valid
RWBuffer<float4> r3; // valid
RWBuffer<notComplete> r4; // invalid - the RET isn't complete, the definition is missing.
// the type trait that would catch this is the negation of `__is_intangible`
RWBuffer<oneInt> r5; // valid - all leaf types are valid primitive types, and homogenous
RWBuffer<oneFloat> r6; // valid
RWBuffer<twoInt> r7; // valid
RWBuffer<threeInts> r8; // valid
RWBuffer<notHomogenous> r9; // invalid, all template type components must have the same type, DXC fails
StructuredBuffer<notHomogenous> r9Structured; // valid
RWBuffer<depthDiff> r10; // invalid, all template type components must have the same type, DXC fails
RWBuffer<EightElements> r11; // invalid, > 4 elements and > 16 bytes, DXC fails
// This would be caught by __builtin_hlsl_is_line_vector_layout_compatible
StructuredBuffer<EightElements> r9Structured; // valid
RWBuffer<EightHalves> r12; // invalid, > 4 elements, DXC fails
// This would be caught by __builtin_hlsl_is_line_vector_layout_compatible
StructuredBuffer<EightHalves> r12Structured; // valid
RWBuffer<oneIntWithVec> r13; // valid
RWBuffer<weirdStruct> r14; // valid
RWBuffer<RWBuffer<int> > r15; // invalid - the RET has a handle with unknown size, thus it is an intangible RET.
// the type trait that would catch this is the negation of `__is_intangible`
```
damyanp marked this conversation as resolved.
Show resolved Hide resolved

Below is a sample C++ implementation of the `RWBuffer` resource type, which is a typed buffer variant.
This code would exist within an hlsl header, but concepts are not implemented in HLSL. Instead, the AST node
associated with RWBuffers is constructed as if this code was read and parsed by the compiler.
```
#include <type_traits>

namespace hlsl {

template<typename T>
damyanp marked this conversation as resolved.
Show resolved Hide resolved
concept is_valid_line_vector =
__builtin_hlsl_is_line_vector_layout_compatible<T>();

template<typename element_type> requires !__is_intangible(element_type) && is_valid_line_vector<element_type>
struct RWBuffer {
element_type Val;
};

// doesn't need __builtin_hlsl_is_line_vector_layout_compatible, because this is a raw buffer
// also, raw buffers allow bools and enums as constituent types
template<typename T> requires !__is_intangible(T)
struct StructuredBuffer {
T Val;
};
}

```

## Alternatives considered (Optional)
We could instead implement a diagnostic function that checks each of these conceptual constraints in
one place, either in Sema or CodeGen, but this would prevent us from defining a single header where
all resource information is localized.

Another alternative considered was creating a builtin called `__is_valid_resource_element_type`, to
check all possible valid resource element types, rather than just checking that the RET is not intangible.
This is unneeded because all primitive non-intangible types are valid RETs.

## Acknowledgments (Optional)
* Damyan Pepper
* Chris Bieneman
* Greg Roth
* Sarah Spall
* Tex Riddell
* Justin Bogner
<!-- {% endraw %} -->