[HLSL] Add empty struct test cases to `__builtin_hlsl_is_typed_resource_element_compatible` test file #115045

bob80905 · 2024-11-05T18:37:17Z

This PR adds empty struct cases to the test file for the builtin.

llvmbot · 2024-11-05T18:37:51Z

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Joshua Batista (bob80905)

Changes

This PR adds empty struct cases to the test file for the builtin.

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

1 Files Affected:

(modified) clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl (+2)

diff --git a/clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl b/clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl
index acc1f281daddfc..08d75a0c23b228 100644
--- a/clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl
+++ b/clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl
@@ -107,3 +107,5 @@ struct TypeDefTest {
 };
 
 _Static_assert(__builtin_hlsl_is_typed_resource_element_compatible(TypeDefTest), "");
+
+

llvm-beanz

I think there is actually a bigger problem with your earlier PR that we need to address.

There is an inconsistency in the design document. Specifically in the beginning of the document it says:

Element types 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

Should be a scalar or homogenous 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

(edit: fixed formatting on quoted text, the formatting error is in the original document)

By this, elements should either be scalar or vectors, but not struct types.

The document later goes on to list test cases that are struct types.

I believe we had determined that DXC generates invalid code for some (most? all?) cases of structs in typed buffer elements, so we had determined to not support structs as element types in Clang.

bob80905 · 2024-11-06T00:26:45Z

I believe we had determined that DXC generates invalid code for some (most? all?) cases of structs in typed buffer elements, so we had determined to not support structs as element types in Clang.

Correct, I'll edit the spec to reflect this more clearly. I've removed most of the struct tests.

llvm-beanz · 2024-11-07T17:52:34Z

Correct, I'll edit the spec to reflect this more clearly. I've removed most of the struct tests.
I don't think that solves this problem. The code as implemented doesn't match what it should be doing. You shouldn't need to build the flattened type list if only vectors and scalars are allowed so we should be able to simplify this implementation.

clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl

coopp

Looks good to me.

llvm-beanz · 2024-11-08T21:00:50Z

clang/lib/Sema/SemaHLSL.cpp

+  if (CanonicalType->getAs<clang::RecordType>()) {
    return false;
+  }


nit: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Suggested change

if (CanonicalType->getAs<clang::RecordType>()) {

return false;

}

if (CanonicalType->getAs<clang::RecordType>())

return false;

llvm-beanz · 2024-11-08T21:06:24Z

clang/lib/Sema/SemaHLSL.cpp

-  llvm::SmallVector<QualType, 4> QTTypes;
-  BuildFlattenedTypeList(QT, QTTypes);
+  // the only other valid builtin types are scalars or vectors
+  if (const BuiltinType *BT = CanonicalType->getAs<BuiltinType>()) {


Vectors aren't BuitinType, so this can probably be simplified. Something like:

// All arithmetic and enumeration types are valid. if (QT->isArithmeticType() || QT->isEnumeralType()) return true;

llvm-beanz · 2024-11-08T21:10:30Z

clang/lib/Sema/SemaHLSL.cpp

+    QualType ElTy = VT->getElementType();
+    int TotalSizeInBytes = (SemaRef.Context.getTypeSize(ElTy) / 8) * ArraySize;
+
+    if (TotalSizeInBytes > 16)


Can this be simplified to:

Suggested change

QualType ElTy = VT->getElementType();

int TotalSizeInBytes = (SemaRef.Context.getTypeSize(ElTy) / 8) * ArraySize;

if (TotalSizeInBytes > 16)

if (SemaRef.Context.getTypeSize(QT) > 16)

With a division by 8, yes that is a good simplification

llvm-beanz · 2024-11-08T21:11:15Z

clang/lib/Sema/SemaHLSL.cpp

-  // check if the outer type was an array type
-  if (QT->isArrayType())
+  // UDT types are not allowed
+  clang::QualType CanonicalType = QT.getCanonicalType();


Most of the ways you're actually using this you likely don't need to query the canonical type. QT->isRecordType() will query the type of the canonical type directly.

llvm-beanz · 2024-11-08T21:11:50Z

clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl


-_Static_assert(__builtin_hlsl_is_typed_resource_element_compatible(TypeDefTest), "");


Do we need to remove all these tests, or just update them to expect false results?

They could be left in and switched to false, but it doesn't seem valuable. All structs are disallowed, so I think just one struct test would suffice.

llvm-beanz · 2024-11-13T21:45:18Z

clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl

-_Static_assert(__builtin_hlsl_is_typed_resource_element_compatible(SimpleTemplate<__hlsl_resource_t>), "");
-
-_Static_assert(__builtin_hlsl_is_typed_resource_element_compatible(SimpleTemplate<float>), "");
+// structs not allowed


This change may be a little too aggressive at removing test cases. All the struct cases should return false, but we could maybe test a few different struct formations. Some key ones to cover: forward declaration (incomplete), empty structure, template struct, template struct containing a vector that would be valid.

llvm-beanz · 2024-11-13T21:46:13Z

clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl


-typedef int myInt;
+typedef vector<int, 8> int8;


We should also have typedef tests that fail, the only one you have right now is a successful case.

What about arrays?

_Static_assert(!__builtin_hlsl_is_typed_resource_element_compatible(float[3]), "");

The typedef test that I have right now does return false when evaluated by the builtin, it's a typedef of 8 ints in a vector. I also test for arrays with half[4]. Are these sufficient?

bogner · 2024-11-15T01:56:46Z

When you change what a PR does, please remember to update the title and description. This went in saying it's about adding some test cases but it actually changes much more than that.

add empty struct test cases

ef4a7ee

llvmbot added clang Clang issues not falling into any other category HLSL HLSL Language Support labels Nov 5, 2024

bob80905 mentioned this pull request Nov 5, 2024

[HLSL] add IsTypedResourceElementCompatible type trait #114864

Merged

remove assert, return false instead

0ed4809

llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Nov 5, 2024

hekota approved these changes Nov 5, 2024

View reviewed changes

llvm-beanz requested changes Nov 5, 2024

View reviewed changes

address chris

65c6e1a

address chris, remove homogeneity check and buildFlattenedList call

3456c14

coopp reviewed Nov 8, 2024

View reviewed changes

clang/test/SemaHLSL/Types/Traits/IsTypedResourceElementCompatible.hlsl Show resolved Hide resolved

coopp approved these changes Nov 8, 2024

View reviewed changes

llvm-beanz reviewed Nov 8, 2024

View reviewed changes

simplify logic, address chris

c912bfd

llvm-beanz reviewed Nov 13, 2024

View reviewed changes

bob80905 added 2 commits November 13, 2024 14:32

add more test cases

90a73b8

add bool and enum cases, including vector of bools

b3b8afb

llvm-beanz approved these changes Nov 15, 2024

View reviewed changes

bob80905 merged commit 47889cd into llvm:main Nov 15, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HLSL] Add empty struct test cases to `__builtin_hlsl_is_typed_resource_element_compatible` test file #115045

[HLSL] Add empty struct test cases to `__builtin_hlsl_is_typed_resource_element_compatible` test file #115045

bob80905 commented Nov 5, 2024

llvmbot commented Nov 5, 2024 •

edited

Loading

llvm-beanz left a comment •

edited

Loading

bob80905 commented Nov 6, 2024

llvm-beanz commented Nov 7, 2024

coopp left a comment

llvm-beanz Nov 8, 2024

llvm-beanz Nov 8, 2024

llvm-beanz Nov 8, 2024

bob80905 Nov 8, 2024

llvm-beanz Nov 8, 2024

llvm-beanz Nov 8, 2024

bob80905 Nov 8, 2024

llvm-beanz Nov 13, 2024

llvm-beanz Nov 13, 2024

bob80905 Nov 13, 2024

bogner commented Nov 15, 2024


		_Static_assert(__builtin_hlsl_is_typed_resource_element_compatible(TypeDefTest), "");

[HLSL] Add empty struct test cases to __builtin_hlsl_is_typed_resource_element_compatible test file #115045

[HLSL] Add empty struct test cases to __builtin_hlsl_is_typed_resource_element_compatible test file #115045

Conversation

bob80905 commented Nov 5, 2024

llvmbot commented Nov 5, 2024 • edited Loading

llvm-beanz left a comment • edited Loading

Choose a reason for hiding this comment

bob80905 commented Nov 6, 2024

llvm-beanz commented Nov 7, 2024

coopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogner commented Nov 15, 2024

[HLSL] Add empty struct test cases to `__builtin_hlsl_is_typed_resource_element_compatible` test file #115045

[HLSL] Add empty struct test cases to `__builtin_hlsl_is_typed_resource_element_compatible` test file #115045

llvmbot commented Nov 5, 2024 •

edited

Loading

llvm-beanz left a comment •

edited

Loading