Skip to content

Add offload-test-suite support for ldexp #200

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kmpeng
Copy link
Contributor

@kmpeng kmpeng commented May 20, 2025

Closes #95.

  • Test for Half type (ldexp.fp16.test)
  • Test for 32 bit types (ldexp.32.test)

@alsepkow
Copy link

Should 'ldexp.32.test' be something like 'ldexp.fp32.test' instead?

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

StructuredBuffer<float4> In : register(t0);

Choose a reason for hiding this comment

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

Is there a reason we use float4 instead of float?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This way each version can easily be tested. If it was float, a float4, etc, would need to be constructed.
Line 8 tests the float4 version of ldexp, like 10 tests float3 etc

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

StructuredBuffer<float4> In : register(t0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This way each version can easily be tested. If it was float, a float4, etc, would need to be constructed.
Line 8 tests the float4 version of ldexp, like 10 tests float3 etc

Out[1].x = ldexp(In[1].x, In[1].y);
Out[1].yzw = ldexp(In[1].yzw, In[0].yzw);
Out[2].xy = ldexp(In[0].xy, In[1].yw);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be helpful to add a comment showing what value combinations are being tested; I believe this is correct (but the formatting is messed up)

// Num       Exp
// 3.14159  1
// 0             1.5
// -inf          -0.5
// Nan         inf
// 3.14159   0
// 1.5           0
// -.05         -inf
// inf           Nan
// 3.14159  1.5
// 0             inf

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 this in the main function but didn't know if that made sense. Lmk if you think there's a better place to move it

@kmpeng
Copy link
Contributor Author

kmpeng commented May 21, 2025

Should 'ldexp.32.test' be something like 'ldexp.fp32.test' instead?

I was also wondering this and think it would make more sense if it were ldexp.fp32.test. I was following the issue description that said to name the test for 32 bits ldexp.32.test - didn't know if it was written that way for a reason. @spall Any thoughts?

@spall
Copy link
Collaborator

spall commented May 22, 2025

Should 'ldexp.32.test' be something like 'ldexp.fp32.test' instead?

I was also wondering this and think it would make more sense if it were ldexp.fp32.test. I was following the issue description that said to name the test for 32 bits ldexp.32.test - didn't know if it was written that way for a reason. @spall Any thoughts?

Since not all GPUs support all bit sizes, or all types, the idea was to separate the types we need to check for gpu capability for in separate files. So a function which supports half and int16 would have both a fp16.test and int16.test file because those would be separate capabilities we need to check for. For 32 bit types, they should be supported by all GPUs, so they would go into one file 32.test. Since ldexp operates on only half and float I think the better change would be to name the 16 bit test file ldexp.16.test, to make it clear there isn't a missing int16.test file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test for ldexp
3 participants