Skip to content

Conversation

@joaosaffran
Copy link
Collaborator

@joaosaffran joaosaffran commented Oct 24, 2025

This PR adds isfinite intrinsic test to long vectors.

Closes: #7850

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@joaosaffran joaosaffran marked this pull request as ready for review October 27, 2025 19:16
@joaosaffran joaosaffran requested a review from alsepkow October 27, 2025 19:21
INPUT_SET(InputSet::Positive, 1.0f, 1.0f, 65535.0f, 0.01f, 5531.0f, 0.01f, 1.0f,
0.01f, 331.2330f, 3250.01f);
INPUT_SET(InputSet::SelectCond, 0.0f, 1.0f);
INPUT_SET(InputSet::Infinite, std::numeric_limits<float>::infinity(),
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to add some finite values to the set of values we test. Otherwise, all the results are just going to be false.

Copy link
Member

Choose a reason for hiding this comment

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

Looking forward a bit, we know that we're going to be adding some more special values? Would it be better to have an input set that contains a mixture of non-special and special float values?

INPUT_SET(InputSet::Positive, 1.0, 1.0, 342.0, 0.01, 5531.0, 0.01, 1.0, 0.01,
331.2330, 3250.01);
INPUT_SET(InputSet::SelectCond, 0.0, 1.0);
INPUT_SET(InputSet::Infinite, std::numeric_limits<HLSLHalf_t>::infinity(),
Copy link
Contributor

Choose a reason for hiding this comment

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

You should double check what's happening here. Passing HLSLHalf_t as the template argument to std::numeric_limits will be doing an implicit conversion to float. Which is fine. And it looks like DirectX::PackedVector::XMConvertFloatToHalf does the right thing by converting that to an inf 'HALF'.

I think it would be better to do std::numeric_limits::infinity() to be more explicit. What do you think?

// Float Ops
//

#define BOOLEAN_FLOAT_OP(OP, IMPL) \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this macro will be helpful for the other float ops we're adding.

Copy link
Member

Choose a reason for hiding this comment

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

"boolean float op" sounds a bit contradictory.

I think we're doing a bunch of "float specials" here, I wonder if it would make sense to group them in that way?

Copy link
Contributor

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

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

Some minor suggestions. Outside of those LGTM

@damyanp
Copy link
Member

damyanp commented Oct 28, 2025

Please associate this with the appropriate workitem (maybe with a "closes #xxx" type thing in the PR description)

@joaosaffran joaosaffran linked an issue Oct 28, 2025 that may be closed by this pull request
OP(Ternary, Select, 3, "TestSelect", "", " -DFUNC_TEST_SELECT=1",
"LongVectorOp", SelectCond, Default2, Default3)

OP(Unary, IsFinite, 1, "TestIsFinite", "", " -DFUNC_TEST_ISFINITE=1",
Copy link
Member

Choose a reason for hiding this comment

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

Is Unary the right group to use here? Why not UnaryMath (and if so, should move this up next to the other unary math ops)

INPUT_SET(InputSet::Positive, 1.0f, 1.0f, 65535.0f, 0.01f, 5531.0f, 0.01f, 1.0f,
0.01f, 331.2330f, 3250.01f);
INPUT_SET(InputSet::SelectCond, 0.0f, 1.0f);
INPUT_SET(InputSet::Infinite, std::numeric_limits<float>::infinity(),
Copy link
Member

Choose a reason for hiding this comment

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

Looking forward a bit, we know that we're going to be adding some more special values? Would it be better to have an input set that contains a mixture of non-special and special float values?

// Float Ops
//

#define BOOLEAN_FLOAT_OP(OP, IMPL) \
Copy link
Member

Choose a reason for hiding this comment

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

"boolean float op" sounds a bit contradictory.

I think we're doing a bunch of "float specials" here, I wonder if it would make sense to group them in that way?

Comment on lines +1496 to 1497
HLK_TEST(IsFinite, double);
// Explicit Cast
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HLK_TEST(IsFinite, double);
// Explicit Cast
HLK_TEST(IsFinite, double);
// Explicit Cast

}
#endif
#ifdef FUNC_TEST_ISFINITE
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be something like "FLOAT_SPECIALS" and have another define choose between isinifite, isinf and isnan?

@joaosaffran joaosaffran marked this pull request as draft October 28, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

Implement tests for isfinite, isinf and isnan Long Vector Execution Tests: Float Specific Ops

3 participants