Add `distance` tests #270

bob80905 · 2025-06-21T00:56:18Z

This PR adds tests for the distance function. Adds float16 and float32 test files.
Fixes #175

bob80905 · 2025-06-21T00:58:58Z

Should I add a test case for NaNs? Or other non-normal floats?

spall

Yeah adding tests for odd values like NaN and Inf would be good.

tex3d · 2025-06-23T21:31:18Z

test/Feature/HLSLLib/distance.32.test

+  float R3 = distance(X[0].xyzw, Y[0].xyzw);
+  Result[3] = R3;
+
+  // test denormal cases


These aren't denormal (aka. subnormal) cases, which would also be good to test. That would be very small positive or negative values, below the smallest normalized value in the given float representation.

Elsewhere we use the terminology "floating point specials" to refer to INF or NaN non-numeric values.

https://github.com/llvm/offload-test-suite/pull/268/files
you can look here for examples of 16 and 32 bit float values which should be denorm according to c++

tex3d · 2025-07-09T18:44:17Z

test/Feature/HLSLLib/distance.32.denormal.test

+void main() {
+  // test special cases
+  // distance(inf, 1.0) = inf
+  Result[0] = distance(X[0].x, Y[0].x);


The degenerate scalar case is not that interesting for distance, since it just translates to fsub. Vector distance translates to fsub, fmul+fadd per component, sqrt.

So these scalar cases are really only testing the behavior of fsub.

spall · 2025-07-09T19:48:12Z

test/Feature/HLSLLib/distance.fp16.test

+...
+#--- end
+
+# UNSUPPORTED: Clang-Vulkan


Why is Clang-Vulkan unsupported? Sorry if this has been explained before and I forgot or missed it.

# | Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-08742 ] # | vkCreateShaderModule(): SPIR-V Extension SPV_KHR_float_controls2 was declared, but one of the following requirements is required (1.4.0 (0x00404000) or VK_KHR_shader_float_controls2).

is what I get when running it without that line.

Can you leave a comment about that in the tests?

alsepkow · 2025-07-12T02:23:36Z

test/Feature/HLSLLib/distance.32.denormal.test

+  - Name: X
+    Format: Float32
+    Stride: 16
+    Data: [ inf, NaN, -inf, 0 ]


Would -0 also an interesting value to test?

It would, looks like it didn't affect any of the expected results however.

tex3d

I feel like there needs to be an adjustment in approach due to the intended purpose of these tests.

I don't think we need NaN/Inf/Denorm tests
Values used should test full operation/expansion within reasonable ranges and precisions for confidence that the expansion used is correct, but not hyper-sensitive to precision
Don't just test degenerate edge cases that could pass with an incomplete expansion
ULP tolerance should be higher to allow for valid variances in operation expansions
Need constant (literal) input cases to test constant folding

tex3d · 2025-07-15T20:50:19Z

test/Feature/HLSLLib/distance.32.denormal.test

I'm not sure this particular test is necessary, since the purpose of this test suite is to make sure Clang produces the same key DXIL / SPIR-V code as DXC to implement the operation.

The important aspects to test would be the forms of HLSL that generate different operations/dimensions, and a variety of reasonable inputs to verify that the operations performed are the same.

I thought edge cases like these aren't what we care about for this test suite.

Additionally, the denorm tests (after which this file is apparently named) aren't even interesting, as their results would be identical if those denorms were replaced with 0's.

tex3d · 2025-07-15T20:56:11Z

test/Feature/HLSLLib/distance.32.test

+  // distance({0.0, 0.0, 0.0}, {3.0, 4.0, 12.0}) = 13.0
+  float R2 = distance(X[0].xyz, Y[0].xyz);
+  Result[2] = R2;
+  // distance({0.0, 0.0, 0.0, 0.0}, {3.0, 4.0, 12.0, 84.0}) = 85.0


Always using 0's for the first argument is an edge case that doesn't even require the full operation expansion (initial subtraction could be omitted, making it the same as length). It would be better to test some random values in reasonable ranges on both sides with a reasonable ULP tolerance for comparison of expected results. That should be sufficient to show that the correct operations are used for each compiler scenario.

Testing the exact precision of implementations isn't the point of this test either, so having a higher tolerance than necessary would probably not be a problem.

Come to think of it, one special case that SHOULD be tested is when inputs are constant. In that case, the compiler (likely) constant-folds operations, which is a separate code path.

Constant inputs should be tested for most/all math ops.

tex3d · 2025-07-15T21:02:30Z

test/Feature/HLSLLib/distance.32.test

+Results:
+  - Result: CheckResult
+    Rule: BufferFloatULP
+    ULPT: 1 


ULP of 1 seems pretty tight. Since the expansion could use dot or fma with varying ordering, and uses sqrt, it seems possible you could exceed the tolerance on a perfectly valid implementation.

test/Feature/HLSLLib/distance.32.test

kmpeng · 2025-07-18T20:33:49Z

test/Feature/HLSLLib/distance.32.test

+  - Name: X
+    Format: Float32
+    Stride: 16
+    Data: [ 1.125, 2.5, 4.75, 6.625 ]


Would it be interesting to test some negative values? (same for the half test)

kmpeng · 2025-07-18T20:42:09Z

test/Feature/HLSLLib/distance.32.test

+  Result[5] = R2_constant;
+
+  // distance({1.125, 2.5, 4.75, 6.625}, {2.375, 5.25, 8.375, 5.30}) = 4.90115
+  float R3 = distance(X[0].xyzw, Y[0].xyzw);


NIT: you don't need to do X[0].xyzw and Y[0].xyzw, you can just do distance(X[0], Y[0]) (same for the half test)

kmpeng · 2025-07-18T20:43:16Z

test/Feature/HLSLLib/distance.fp16.test

+    Data: [ 0x3c80, 0x4100, 0x44c0, 0x46a0 ]
+  - Name: Y


I think it would be useful to add comments under the Data lines listing these values, even though you have them commented in main so you don't have to scroll up to look for/match them

Data: [ 0x3c80, 0x4100, 0x44c0, 0x46a0 ] # 1.125, 2.5, 4.75, 6.625

kmpeng · 2025-07-18T20:50:34Z

test/Feature/HLSLLib/distance.fp16.test

@@ -0,0 +1,100 @@
+#--- source.hlsl


I remember for ldexp, @spall said to just name my files <intrinsic>.32.test and <intrinsic>.16.test to make it clear there isn't a missing int16.test file. Probably want her input for this though

spall reviewed Jun 23, 2025

View reviewed changes

bob80905 force-pushed the add_distance_tests branch from 5cb1a01 to 913c363 Compare June 23, 2025 17:50

tex3d reviewed Jun 23, 2025

View reviewed changes

bob80905 force-pushed the add_distance_tests branch from be926bc to ae5988a Compare July 3, 2025 00:53

bob80905 added 7 commits July 7, 2025 15:37

add distance tests

9540337

add denormal test cases

9e0aab2

use different representation for NaN float16s

339f9f7

split out non-normal values, add denorm values

71ae411

split out 32 bit special value tests as well

8cc532b

add -Gis

f626d65

try again on a succeeding windows intel machine

8b75db9

bob80905 force-pushed the add_distance_tests branch from 638892e to 8b75db9 Compare July 7, 2025 23:33

bob80905 added 3 commits July 8, 2025 09:02

Merge branch 'main' into add_distance_tests

96cd7bf

delete fp16 special values test

26e7437

Merge branch 'main' into add_distance_tests

cb4779d

tex3d reviewed Jul 9, 2025

View reviewed changes

spall reviewed Jul 9, 2025

View reviewed changes

address scalar comparisons and remove vulkan disallow

977c5fa

bob80905 force-pushed the add_distance_tests branch from 15dded5 to 977c5fa Compare July 9, 2025 20:32

bob80905 added 2 commits July 9, 2025 13:33

Merge branch 'main' into add_distance_tests

966d9d9

add note on why clang-vulkan is unsupported

c5c78c5

alsepkow reviewed Jul 12, 2025

View reviewed changes

add -0.0

414704e

tex3d reviewed Jul 15, 2025

View reviewed changes

incraese ULP, use constant folding tests, use more diverse inputs

ed7e7f7

kmpeng reviewed Jul 16, 2025

View reviewed changes

test/Feature/HLSLLib/distance.32.test Outdated Show resolved Hide resolved

test/Feature/HLSLLib/distance.32.test Outdated Show resolved Hide resolved

bob80905 added 3 commits July 16, 2025 15:54

address Kaitlin

f081c5d

fix a copy paste mistake

36cf096

add top level test comment

e5b53c4

kmpeng reviewed Jul 18, 2025

View reviewed changes

address Kaitlin

d056bd1

Add distance tests #270

Are you sure you want to change the base?

Add distance tests #270

Conversation

bob80905 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bob80905 commented Jun 21, 2025

Uh oh!

spall 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!

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!

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!

Uh oh!

Uh oh!

kmpeng Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Add `distance` tests #270

Add `distance` tests #270

bob80905 commented Jun 21, 2025 •

edited

Loading

kmpeng Jul 18, 2025 •

edited

Loading