Adding array operator long vector tests to HLK #7887

joaosaffran · 2025-11-07T21:56:11Z

This patch is adding array operator long vector test to HLK. There are 3 scenarios that were identified when doing those tests:

Dynamic Element Access: the generated dxil uses a GEP instruction to calculate the correct address to access.
Static element access: the code uses extracelement and insertelement.

Closes: #7618

damyanp · 2025-11-10T23:03:46Z

/azp run

azure-pipelines · 2025-11-10T23:03:55Z

Azure Pipelines successfully started running 1 pipeline(s).

tools/clang/unittests/HLSLExec/LongVectorOps.def

tools/clang/unittests/HLSLExec/ShaderOpArith.xml

tex3d · 2025-11-14T23:34:51Z

tools/clang/unittests/HLSLExec/ShaderOpArith.xml

+              OutputVector[IndexList[i]] = Input1[IndexList[i]];
+              uint index = (uint)(IndexList[i]);


Did you mean this? Otherwise index is not used.

Suggested change

OutputVector[IndexList[i]] = Input1[IndexList[i]];

uint index = (uint)(IndexList[i]);

uint index = (uint)(IndexList[i]);

OutputVector[index] = Input1[index];

tex3d · 2025-11-14T23:38:36Z

tools/clang/unittests/HLSLExec/ShaderOpArith.xml

+            uint Modifier = (uint) Input2[0];
+            uint IndexList[6] = {0 + Modifier, OutNum - 1 + Modifier, 1 + Modifier, OutNum - 2 + Modifier, OutNum / 2 + Modifier, OutNum / 2 + 1 + Modifier};


Typically you can use the name Zero for clarity. Also, if you add this Zero to the initialization of index below instead of the init list, the code would be a bit simpler, and equivalent. Then it's easier to compare IndexList between static/dynamic cases. If you had a common location, you could even share the code.

tex3d · 2025-11-14T23:48:55Z

tools/clang/unittests/HLSLExec/ShaderOpArith.xml


-          #if IS_UNARY_OP
+          #if TEST_ARRAY_OPERATOR_STATIC_ACCESS
+            uint IndexList[6] = {0, OutNum - 1, 1, OutNum - 2, OutNum / 2, OutNum / 2 + 1};


The count could be a constant, then used later instead of 6 for clarity.
Plus the IndexList could be const and formatted to make it easier to see the pattern.

Suggested change

uint IndexList[6] = {0, OutNum - 1, 1, OutNum - 2, OutNum / 2, OutNum / 2 + 1};

const uint IndexCount = 6;

const uint IndexList[IndexCount] = {

0, OutNum - 1,

1, OutNum - 2,

OutNum / 2, OutNum / 2 + 1

};

tex3d · 2025-11-14T23:49:28Z

tools/clang/unittests/HLSLExec/ShaderOpArith.xml

+            vector<OUT_TYPE, OutNum> OutputVector = 0;
+            uint End = min(OutNum, 6);
+
+            [unroll]for(uint i = 0; i < 6; ++i) {


Loop uses 6 instead of End.

Suggested change

[unroll]for(uint i = 0; i < 6; ++i) {

[unroll]for(uint i = 0; i < End; ++i) {

tex3d · 2025-11-14T23:54:58Z

tools/clang/unittests/HLSLExec/LongVectorTestData.h

  HLSLBool_t() : Val(0) {}
  HLSLBool_t(int32_t Val) : Val(Val) {}
  HLSLBool_t(bool Val) : Val(Val) {}
+  explicit HLSLBool_t(float Val) : Val(Val) {}


Why is adding this operator necessary? I don't see where it's used.

tex3d · 2025-11-15T00:08:50Z

tools/clang/unittests/HLSLExec/LongVectors.cpp

+
+  size_t IndexList[6] = {
+      0, VectorSize - 1, 1, VectorSize - 2, VectorSize / 2, VectorSize / 2 + 1};
+  for (size_t i = 0; i < 6; ++i)


I suppose if you're going to use min(VectorSize, 6) in HLSL for the loop count, you should do so here too. The only difference for vectors of length 3 to 5 is redundant operations on short vectors. If you use a vector length of 1 or 2, this will index out-of-bounds at VectorSize - 2 for length 1 and VectorSize / 2 + 1 for lengths 1 and 2.

damyanp

I think this LGTM. It took me quite a while to try and figure out what these tests were doing. I wonder if there are some short, directed, comments that could help explain things?

alsepkow

LGTM. A few minor nits and a suggestion to avoid comparing a bunch of extra zeros.

alsepkow · 2025-11-17T21:41:41Z

tools/clang/unittests/HLSLExec/LongVectors.cpp

+  size_t IndexList[IndexCount] = {
+      0, VectorSize - 1, 1, VectorSize - 2, VectorSize / 2, VectorSize / 2 + 1};
+  size_t End = std::min(VectorSize, IndexCount);
+  for (size_t i = 0; i < End; ++i)


casing, i should be I

alsepkow · 2025-11-17T22:17:35Z

tools/clang/unittests/HLSLExec/ShaderOpArith.xml

+          #if TEST_ARRAY_OPERATOR
+            // This test case is for testing array operator [].
+            // It tests static array access with a compile time constant index array.
+            // And dynamic access, by introducing a runtime dependency that prevents the


very minor nit: Change 'And' to 'Or' to further clarify its one or the other at a time.

Suggested change

// And dynamic access, by introducing a runtime dependency that prevents the

// Or dynamic access, by introducing a runtime dependency that prevents the

alsepkow · 2025-11-17T22:18:47Z

tools/clang/unittests/HLSLExec/ShaderOpArith.xml

+            uint End = min(OutNum, IndexCount);
+
+            #if DYNAMIC_ACCESS
+              uint Zero = (uint) Input2[0];


alsepkow · 2025-11-17T22:46:53Z

tools/clang/unittests/HLSLExec/LongVectors.cpp

+static std::vector<T> buildExpectedArrayAccess(const InputSets<T> &Inputs) {
+  const size_t VectorSize = Inputs[0].size();
+  std::vector<T> Expected;
+  Expected.resize(VectorSize);


Is there a reason you opted to zero an expected vector for the elements we aren't testing vs just setting the expected output size to 6?

You could avoid verifying a bunch of zeros by doing that. And if I recall correctly, all you will need to do is set the size of Expected to 6 here. The framework we built dispatches calls to other functions based on the number of elements in the Expected vector. And you can set OutNum in the shader to 6 with logic similar to how it's changed for the reduction op.

The test read and write at different indexes on which vector size. This is done to provide GEP instruction with index to access at the beginning, middle and end of vector. Forcing that to be 6 means we will need to store the that at I always, making the storing of elements to always be static.

I could make the IndexVector to be 0 to 5, but that reduces the scope where we test GEP calc for larger vectors

Great point! I wasn't considering the storing portion when I asked that. I wonder if a comment saying that is helpful for a little documentation on that being the intent.

tex3d

LGTM!

joaosaffran · 2025-11-18T18:23:06Z

/azp run

azure-pipelines · 2025-11-18T18:23:18Z

Azure Pipelines successfully started running 1 pipeline(s).

joaosaffran · 2025-11-20T01:11:02Z

/azp run

azure-pipelines · 2025-11-20T01:11:11Z

Azure Pipelines successfully started running 1 pipeline(s).

alsepkow

lgtm

joaosaffran added 7 commits November 3, 2025 17:29

first test

8d371f1

adding single element access test

5cc409e

format

8256850

adding more test

33a888e

clean up

be253dd

foramt

ce5ebdf

clean up

c657ded

github-project-automation bot added this to HLSL Roadmap Nov 7, 2025

github-project-automation bot moved this to New in HLSL Roadmap Nov 7, 2025

damyanp reviewed Nov 10, 2025

View reviewed changes

tools/clang/unittests/HLSLExec/LongVectorOps.def Outdated Show resolved Hide resolved

damyanp reviewed Nov 10, 2025

View reviewed changes

tools/clang/unittests/HLSLExec/ShaderOpArith.xml Outdated Show resolved Hide resolved

damyanp reviewed Nov 10, 2025

View reviewed changes

tools/clang/unittests/HLSLExec/ShaderOpArith.xml Outdated Show resolved Hide resolved

joaosaffran added 2 commits November 13, 2025 15:19

ongoing tests

ab7707f

finishing dynamic access test

a5918e7

joaosaffran force-pushed the test/array-operator branch from eb5a56c to a5918e7 Compare November 14, 2025 22:02

joaosaffran added 5 commits November 14, 2025 14:12

clean up

ad76246

clean up

d4072c1

clean up

c21f0be

Merge branch 'main' into test/array-operator

4deea4a

clean up

4e0fda9

joaosaffran marked this pull request as ready for review November 14, 2025 22:18

joaosaffran requested review from alsepkow, damyanp and tex3d November 14, 2025 22:20

tex3d reviewed Nov 14, 2025

View reviewed changes

tex3d reviewed Nov 15, 2025

View reviewed changes

damyanp approved these changes Nov 15, 2025

View reviewed changes

addressing comments from Tex

22f0c80

adding comments

de0cbc4

joaosaffran requested a review from tex3d November 17, 2025 20:48

joaosaffran added 3 commits November 17, 2025 13:16

foramt

1a8fb8a

fix oopsy

34bb415

fix typo

da88c6a

joaosaffran requested a review from damyanp November 17, 2025 21:30

damyanp approved these changes Nov 17, 2025

View reviewed changes

alsepkow approved these changes Nov 17, 2025

View reviewed changes

addressing comments from Alex

3c32ba5

tex3d approved these changes Nov 18, 2025

View reviewed changes

joaosaffran added 2 commits November 17, 2025 18:09

Merge branch 'main' into test/array-operator

3cc3f31

fix merge

234a466

joaosaffran requested review from alsepkow and damyanp November 20, 2025 18:37

alsepkow approved these changes Nov 20, 2025

View reviewed changes

joaosaffran merged commit 10a4f01 into microsoft:main Nov 20, 2025
12 checks passed

github-project-automation bot moved this from New to Done in HLSL Roadmap Nov 20, 2025

		OutputVector[IndexList[i]] = Input1[IndexList[i]];
		uint index = (uint)(IndexList[i]);

		uint Modifier = (uint) Input2[0];
		uint IndexList[6] = {0 + Modifier, OutNum - 1 + Modifier, 1 + Modifier, OutNum - 2 + Modifier, OutNum / 2 + Modifier, OutNum / 2 + 1 + Modifier};

	[unroll]for(uint i = 0; i < 6; ++i) {
	[unroll]for(uint i = 0; i < End; ++i) {

	// And dynamic access, by introducing a runtime dependency that prevents the
	// Or dynamic access, by introducing a runtime dependency that prevents the

Adding array operator long vector tests to HLK #7887

Adding array operator long vector tests to HLK #7887

Conversation

joaosaffran commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damyanp commented Nov 10, 2025

Uh oh!

azure-pipelines bot commented Nov 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

alsepkow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tex3d left a comment

Choose a reason for hiding this comment

Uh oh!

joaosaffran commented Nov 18, 2025

Uh oh!

azure-pipelines bot commented Nov 18, 2025

Uh oh!

joaosaffran commented Nov 20, 2025

Uh oh!

azure-pipelines bot commented Nov 20, 2025

Uh oh!

alsepkow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joaosaffran commented Nov 7, 2025 •

edited

Loading