Adding ModF tests to HLK Long Vector unittests by joaosaffran · Pull Request #7859 · microsoft/DirectXShaderCompiler

joaosaffran · 2025-10-30T18:34:01Z

This pr is adding ModF into HLK long vector unittest. It also introduces OP_WITH_OUT_PARAM, to help creating test for intrinsics that require of params. This was inspired by Frexp original test, so this also refactors the test to use the macro as well.

Closes: #7851

alsepkow · 2025-10-31T22:37:14Z

+    static std::vector<TYPE> buildExpected(Op<OpType::OPERATION, TYPE, 1>,     \
+                                           const InputSets<TYPE> &Inputs) {    \
+      DXASSERT_NOMSG(Inputs.size() == 1);                                      \
+      size_t VectorSize = Inputs[0].size();                                    \


nit:

Suggested change

size_t VectorSize = Inputs[0].size(); \

const size_t VectorSize = Inputs[0].size(); \

alsepkow · 2025-10-31T22:45:03Z

 #include "LongVectorOps.def"
 };

+#define OP_WITH_OUT_PARAM(OPERATION, TYPE, IMPL)                               \


Suggested change

#define OP_WITH_OUT_PARAM(OPERATION, TYPE, IMPL) \

#define OP_WITH_OUT_PARAM_=(OPERATION, TYPE, IMPL) \

Given that we're restricting this to unary ops I think we can clarify that by appending the 1 in the name like some of the other macros do

alsepkow · 2025-10-31T22:46:52Z

 #include "LongVectorOps.def"
 };

+#define OP_WITH_OUT_PARAM(OPERATION, TYPE, IMPL)                               \


I also think it would be better to put this macro with the other similar class of macro definitions around line 625.
The #define OP_1" etc

alsepkow · 2025-11-03T20:24:39Z

-    // can leverage the existing logic which verify expected values in a
-    // single vector. We just need to make sure that we organize the output in
-    // the same way in the shader and when we read it back.
+OP_WITH_OUT_PARAM_1(Frexp, float, {


nit: You should update OP_WITH_OUT_PARAM_1 to take the fully qualified name 'OpType::Frexp'. That way we match the pattern of the other macros.

alsepkow · 2025-11-03T20:25:16Z

 #define DEFAULT_OP_2(OP, IMPL) OP_2(OP, DefaultValidation<T>, IMPL)
 #define DEFAULT_OP_3(OP, IMPL) OP_3(OP, DefaultValidation<T>, IMPL)

+#define OP_WITH_OUT_PARAM_1(OPERATION, TYPE, IMPL)                             \


nit: Change 'OPEARTION' to 'OP' to match the pattern of the other macros.

alsepkow

LGTM

damyanp

I'm afraid I think that some more time needs to be taken to find a better abstraction for this that doesn't involve so much preprocessor work.

damyanp · 2025-11-04T23:43:13Z

 #define DEFAULT_OP_2(OP, IMPL) OP_2(OP, DefaultValidation<T>, IMPL)
 #define DEFAULT_OP_3(OP, IMPL) OP_3(OP, DefaultValidation<T>, IMPL)

+#define OP_WITH_OUT_PARAM_1(OP, TYPE, IMPL)                                    \


Putting this much code inside a macro definition makes me uneasy - especially once we start putting blocks of code in the IMPL parameter.

I think it might be worth spending some time trying to find ways to have this be written more in C++ than in the preprocessor.

damyanp · 2025-11-04T23:45:14Z

+      Expected.resize(VectorSize * 2);                                         \
+      for (size_t I = 0; I < VectorSize; ++I) {                                \
+        IMPL                                                                   \


So....there's some kind of extra contract that I think IMPL must know to write two values to Expected, based on I and VectorSize?

As with previous comment, this is something we should be able to make more clear and obvious in the code structure, that doesn't rely on preprocessor substitutions to make it work.

alsepkow · 2025-11-07T00:20:07Z

                                ValidationConfig.Type, VerboseLogging));
 }

+template <typename IN_TYPE, typename OUT_TYPE>


You could simplify this to just use <typename T>
No need to have an in/out type. Not being used.

alsepkow · 2025-11-07T00:26:57Z

 }

+template <typename IN_TYPE, typename OUT_TYPE>
+static std::vector<OUT_TYPE> TransformVector_1(


I think it would improve readability to move this to be defined and declared right above the macro where its used.

alsepkow · 2025-11-07T00:44:21Z

 }

+template <typename IN_TYPE, typename OUT_TYPE>
+static std::vector<OUT_TYPE> TransformVector_1(


nit: I don't think 'TransformVector' is the best name.
I'm wondering if something like 'buildExpectedWithOutParam' or something similar. To better convey what its supposed to do. 'TransformVector' just feels a little too generic to me.

TransformVector strongly implies to me that we're applying a transformation matrix to a vector.

alsepkow · 2025-11-07T00:45:44Z


+template <typename T> struct Op<OpType::ModF, T, 1> : DefaultValidation<T> {};
+
+template <typename T> T TransformModF(const T &Input, T &OutParam);


You could just call this modf. There is no 'Transform' part happening in here.

alsepkow · 2025-11-07T00:49:45Z

-      // So it can conversion between float and int is safe.
-      Expected[I + VectorSize] = static_cast<float>(Exp);
-    }
+float TransformFRexp(const float &Input, float &OutParam) {


You can just call this 'frexp'. No 'Transform' needed.
And functions should start with lowercase.

alsepkow · 2025-11-07T00:53:12Z


+template <typename IN_TYPE, typename OUT_TYPE>
+static std::vector<OUT_TYPE> TransformVector_1(
+    const InputSets<IN_TYPE> &Inputs,


I also think it would be good to add a short comment to this helper just explaining that the store the output parameters in the second half of the output buffer to keep things simple as the significant majority of our tests only need one output buffer. So, we stick with that pattern.

damyanp

Looking through all this, I'm wondering why we don't just follow the same pattern that's used for AsUint_SplitDouble? This is another one-off that has a multiple return values.

Just doing this as a special case saves us from trying to find a way to write this generically, which seems unnecessary since I think that this is the only operation we're dealing with that has one out param like this?

damyanp · 2025-11-07T03:07:15Z

+template <typename IN_TYPE, typename OUT_TYPE>
+static std::vector<OUT_TYPE> TransformVector_1(
+    const InputSets<IN_TYPE> &Inputs,
+    function<OUT_TYPE(const IN_TYPE &, OUT_TYPE &)> TransformFunc) {


Ugh - somewhere there's a using namespace std that shouldn't be there. This should be qualified with std::function - although I'm going to ask that you don't use std::function so this is somewhat moot.

damyanp · 2025-11-07T03:21:51Z

 }

+template <typename IN_TYPE, typename OUT_TYPE>
+static std::vector<OUT_TYPE> TransformVector_1(


TransformVector strongly implies to me that we're applying a transformation matrix to a vector.

damyanp · 2025-11-07T03:28:19Z

+template <typename IN_TYPE, typename OUT_TYPE>
+static std::vector<OUT_TYPE> TransformVector_1(
+    const InputSets<IN_TYPE> &Inputs,
+    function<OUT_TYPE(const IN_TYPE &, OUT_TYPE &)> TransformFunc) {


We shouldn't be using std::function here - you'll notice that it's not used for any of the existing opcodes. A goal here has been to have all of this resolved at compile time, while std::function requires dynamic dispatch.

damyanp · 2025-11-07T21:17:33Z

+  float Exp = 0.0f;
+  float Man = std::modf(float(Input), &Exp);


nit: gonna suggest this before Alex gets a chance to:

Suggested change

float Exp = 0.0f;

float Man = std::modf(float(Input), &Exp);

float Exp = 0.0f;

const float Man = std::modf(float(Input), &Exp);

alsepkow

lgtm

joaosaffran and others added 16 commits October 24, 2025 12:59

adding test?

f54718e

remove input set

b8585b8

adding test

0495fd5

fix

271cb0a

Merge branch 'main' into test/isfinite

20eaa93

adding other numeric types test

27f4e31

clean up

5d66ed8

reduce tests to only float point types

068f4fc

format

c51afa9

add weird values

dbbc868

remove double

eab4ea4

adding other tests

a4de02c

rename input set

57a6be9

adding modf test

66bd46b

Merge branch 'main' into test/modf

cfebdfd

adding macro

b5c78af

github-project-automation Bot added this to HLSL Roadmap Oct 30, 2025

github-project-automation Bot moved this to New in HLSL Roadmap Oct 30, 2025

joaosaffran added 2 commits October 30, 2025 11:38

using sizeof

0aa2c1e

removing merge error

17786f1

joaosaffran requested a review from alsepkow October 30, 2025 18:40

alsepkow reviewed Oct 31, 2025

View reviewed changes

Comment thread tools/clang/unittests/HLSLExec/LongVectorOps.def Outdated

alsepkow reviewed Oct 31, 2025

View reviewed changes

addressing comments from alsepkow

2753e74

joaosaffran requested a review from alsepkow November 3, 2025 18:53

alsepkow reviewed Nov 3, 2025

View reviewed changes

addressing comments

0cfb5bd

joaosaffran requested a review from alsepkow November 3, 2025 20:53

alsepkow approved these changes Nov 3, 2025

View reviewed changes

damyanp self-assigned this Nov 4, 2025

damyanp requested changes Nov 4, 2025

View reviewed changes

github-project-automation Bot moved this from New to In progress in HLSL Roadmap Nov 4, 2025

reducing macro usage, addressing damyan comments

be40a7b

joaosaffran requested review from alsepkow and damyanp November 6, 2025 21:01

alsepkow reviewed Nov 7, 2025

View reviewed changes

damyanp requested changes Nov 7, 2025

View reviewed changes

removing generic code

ad8b7ac

joaosaffran requested review from alsepkow and damyanp November 7, 2025 21:08

damyanp reviewed Nov 7, 2025

View reviewed changes

damyanp approved these changes Nov 7, 2025

View reviewed changes

alsepkow approved these changes Nov 7, 2025

View reviewed changes

joaosaffran merged commit 201a5b5 into microsoft:main Nov 12, 2025
12 checks passed

github-project-automation Bot moved this from In progress to Done in HLSL Roadmap Nov 12, 2025

	size_t VectorSize = Inputs[0].size(); \
	const size_t VectorSize = Inputs[0].size(); \

	#define OP_WITH_OUT_PARAM(OPERATION, TYPE, IMPL) \
	#define OP_WITH_OUT_PARAM_=(OPERATION, TYPE, IMPL) \


		template <typename T> struct Op<OpType::ModF, T, 1> : DefaultValidation<T> {};

		template <typename T> T TransformModF(const T &Input, T &OutParam);

Conversation

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

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!

alsepkow left a comment

Choose a reason for hiding this comment

Uh oh!

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

alsepkow Nov 7, 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!

alsepkow Nov 7, 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!

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

damyanp Nov 7, 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!

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 Oct 30, 2025 •

edited

Loading

alsepkow Nov 7, 2025 •

edited

Loading

alsepkow Nov 7, 2025 •

edited

Loading

damyanp Nov 7, 2025 •

edited

Loading