[HLSL] Implement D3DCOLORtoUBYTE4 intrinsic #122202

Icohedron · 2025-01-09T01:20:41Z

Defines the function D3DCOLORtoUBYTE4 in clang/lib/Headers/hlsl/hlsl_intrinsics.h.
Implements the function D3DCOLORtoUBYTE4 as d3d_color_to_ubyte4 in clang/lib/Headers/hlsl/hlsl_detail.h
Adds a HLSL codegen test to clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl
Adds sema tests to clang/test/SemaHLSL/BuiltIns/D3DCOLORtoUBYTE4-errors.hlsl

github-actions · 2025-01-09T01:20:59Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-01-09T01:21:28Z

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Deric Cheung (Icohedron)

Changes

Fixes #99092.

Defines the function D3DCOLORtoUBYTE4 in clang/lib/Headers/hlsl/hlsl_intrinsics.h.
Implements the function D3DCOLORtoUBYTE4 as d3d_color_to_ubyte4 in clang/lib/Headers/hlsl/hlsl_detail.h
Adds a HLSL codegen test to clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl
Adds error tests to clang/test/SemaHLSL/BuiltIns/D3DCOLORtoUBYTE4-errors.hlsl

Full diff: https://github.com/llvm/llvm-project/pull/122202.diff

4 Files Affected:

(modified) clang/lib/Headers/hlsl/hlsl_detail.h (+8)
(modified) clang/lib/Headers/hlsl/hlsl_intrinsics.h (+17)
(added) clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl (+12)
(added) clang/test/SemaHLSL/BuiltIns/D3DCOLORtoUBYTE4-errors.hlsl (+13)

diff --git a/clang/lib/Headers/hlsl/hlsl_detail.h b/clang/lib/Headers/hlsl/hlsl_detail.h
index 8d5fd941331531..470fa4214a12f8 100644
--- a/clang/lib/Headers/hlsl/hlsl_detail.h
+++ b/clang/lib/Headers/hlsl/hlsl_detail.h
@@ -33,6 +33,14 @@ constexpr enable_if_t<sizeof(U) == sizeof(T), U> bit_cast(T F) {
   return __builtin_bit_cast(U, F);
 }
 
+constexpr vector<uint, 4> d3d_color_to_ubyte4(vector<float, 4> V) {
+  // Use the same scaling factor used by FXC (i.e., 255.001953)
+  // Excerpt from stackoverflow discussion:
+  // "Built-in rounding, necessary because of truncation. 0.001953 * 256 = 0.5"
+  // https://stackoverflow.com/questions/52103720/why-does-d3dcolortoubyte4-multiplies-components-by-255-001953f
+  return V.zyxw * 255.001953f;
+}
+
 } // namespace __detail
 } // namespace hlsl
 #endif //_HLSL_HLSL_DETAILS_H_
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index b745997f1d5a2b..e44403c6c802e0 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -1857,6 +1857,23 @@ half3 cross(half3, half3);
 _HLSL_BUILTIN_ALIAS(__builtin_hlsl_cross)
 float3 cross(float3, float3);
 
+//===----------------------------------------------------------------------===//
+// D3DCOLORtoUBYTE4 builtins
+//===----------------------------------------------------------------------===//
+
+/// \fn T D3DCOLORtoUBYTE4(T x)
+/// \brief Converts a floating-point, 4D vector set by a D3DCOLOR to a UBYTE4.
+/// \param x [in] The floating-point vector4 to convert.
+///
+/// The return value is the UBYTE4 representation of the \a x parameter.
+///
+/// This function swizzles and scales components of the \a x parameter. Use this
+/// function to compensate for the lack of UBYTE4 support in some hardware.
+
+constexpr vector<uint, 4> D3DCOLORtoUBYTE4(vector<float, 4> V) {
+  return __detail::d3d_color_to_ubyte4(V);
+}
+
 //===----------------------------------------------------------------------===//
 // rcp builtins
 //===----------------------------------------------------------------------===//
diff --git a/clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl b/clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl
new file mode 100644
index 00000000000000..6e48800dae6698
--- /dev/null
+++ b/clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -finclude-default-header -triple \
+// RUN:   dxil-pc-shadermodel6.3-library %s -fnative-half-type \
+// RUN:   -emit-llvm -O1 -o - | FileCheck %s --check-prefixes=CHECK
+
+// CHECK-LABEL: D3DCOLORtoUBYTE4
+int4 test_D3DCOLORtoUBYTE4 ( float4 p1 ) {
+  // CHECK: %[[SCALED:.*]] = fmul <4 x float> %{{.*}}, splat (float 0x406FE01000000000)
+  // CHECK: %[[CONVERTED:.*]] = fptoui <4 x float> %[[SCALED]] to <4 x i32>
+  // CHECK: %[[SHUFFLED:.*]] = shufflevector <4 x i32> %{{.*}}, <4 x i32> poison, <4 x i32> <i32 2, i32 1, i32 0, i32 3>
+  // CHECK: ret <4 x i32> %[[SHUFFLED]]
+  return D3DCOLORtoUBYTE4 ( p1 );
+}
diff --git a/clang/test/SemaHLSL/BuiltIns/D3DCOLORtoUBYTE4-errors.hlsl b/clang/test/SemaHLSL/BuiltIns/D3DCOLORtoUBYTE4-errors.hlsl
new file mode 100644
index 00000000000000..fb41c212bd0be3
--- /dev/null
+++ b/clang/test/SemaHLSL/BuiltIns/D3DCOLORtoUBYTE4-errors.hlsl
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.6-library %s -emit-llvm-only -disable-llvm-passes -verify
+
+int4 test_too_few_arg() {
+  return D3DCOLORtoUBYTE4();
+  // expected-error@-1 {{no matching function for call to 'D3DCOLORtoUBYTE4'}}
+  // expected-note@hlsl/hlsl_intrinsics.h:* {{candidate function not viable: requires single argument 'V', but no arguments were provided}}
+}
+
+int4 test_too_many_arg(float4 v) {
+  return D3DCOLORtoUBYTE4(v, v);
+  // expected-error@-1 {{no matching function for call to 'D3DCOLORtoUBYTE4'}}
+  // expected-note@hlsl/hlsl_intrinsics.h:* {{candidate function not viable: requires single argument 'V', but 2 arguments were provided}}
+}

Icohedron · 2025-01-09T19:11:35Z

clang/lib/Headers/hlsl/hlsl_detail.h

+  // Excerpt from stackoverflow discussion:
+  // "Built-in rounding, necessary because of truncation. 0.001953 * 256 = 0.5"
+  // https://stackoverflow.com/questions/52103720/why-does-d3dcolortoubyte4-multiplies-components-by-255-001953f
+  return V.zyxw * 255.001953f;


This implementation of D3DCOLORtoUBYTE4 scales the vector by 255.001953. This matches the way it was done in DXC.
However, it differs from the SPIRV custom implementation which scales the vector by 255.002.

I think the change from DXC for SPIR-V is fine. The DXIL behaviour is the correct behaviour.

inbelic

Great job, LGTM. Just a couple personal preference nits

inbelic · 2025-01-10T04:15:07Z

clang/lib/Headers/hlsl/hlsl_intrinsics.h

@@ -1857,6 +1857,23 @@ half3 cross(half3, half3);
 _HLSL_BUILTIN_ALIAS(__builtin_hlsl_cross)
 float3 cross(float3, float3);

+//===----------------------------------------------------------------------===//
+// D3DCOLORtoUBYTE4 builtins


Suggested change

// D3DCOLORtoUBYTE4 builtins

// D3DCOLORtoUBYTE4 builtin

inbelic · 2025-01-10T04:17:31Z

clang/lib/Headers/hlsl/hlsl_detail.h

@@ -33,6 +33,14 @@ constexpr enable_if_t<sizeof(U) == sizeof(T), U> bit_cast(T F) {
  return __builtin_bit_cast(U, F);
 }

+constexpr vector<uint, 4> d3d_color_to_ubyte4(vector<float, 4> V) {


Suggested change

constexpr vector<uint, 4> d3d_color_to_ubyte4(vector<float, 4> V) {

constexpr vector<uint, 4> D3DColorToUByte4Impl(vector<float, 4> V) {

I think this follows more to how the length builtin was defined

inbelic · 2025-01-10T04:19:02Z

clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl

@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -finclude-default-header -triple \
+// RUN:   dxil-pc-shadermodel6.3-library %s -fnative-half-type \


Suggested change

// RUN: dxil-pc-shadermodel6.3-library %s -fnative-half-type \

// RUN: dxil-pc-shadermodel6.3-library %s \

inbelic · 2025-01-10T04:21:04Z

clang/test/CodeGenHLSL/builtins/D3DCOLORtoUBYTE4.hlsl

+
+// CHECK-LABEL: D3DCOLORtoUBYTE4
+int4 test_D3DCOLORtoUBYTE4(float4 p1) {
+  // CHECK: %[[SCALED:.*]] = fmul [[FMFLAGS:.*]]<4 x float> %{{.*}}, splat (float 0x406FE01000000000)


Suggested change

// CHECK: %[[SCALED:.*]] = fmul [[FMFLAGS:.*]]<4 x float> %{{.*}}, splat (float 0x406FE01000000000)

// CHECK: %[[SCALED:.*]] = fmul [[FMFLAGS:.*]] [[FLOAT_TYPE:<4 x float>]] %{{.*}}, splat (float 0x406FE01000000000)

Imo pattern capturing makes it slightly easier to read and quickly know which types are the same

s-perron · 2025-01-10T15:20:54Z

clang/lib/Headers/hlsl/hlsl_detail.h

+  // Excerpt from stackoverflow discussion:
+  // "Built-in rounding, necessary because of truncation. 0.001953 * 256 = 0.5"
+  // https://stackoverflow.com/questions/52103720/why-does-d3dcolortoubyte4-multiplies-components-by-255-001953f
+  return V.zyxw * 255.001953f;


I think the change from DXC for SPIR-V is fine. The DXIL behaviour is the correct behaviour.

…BYTE4

bogner · 2025-01-13T01:55:18Z

clang/lib/Headers/hlsl/hlsl_detail.h

+  // Use the same scaling factor used by FXC (i.e., 255.001953)
+  // Excerpt from stackoverflow discussion:
+  // "Built-in rounding, necessary because of truncation. 0.001953 * 256 = 0.5"
+  // https://stackoverflow.com/questions/52103720/why-does-d3dcolortoubyte4-multiplies-components-by-255-001953f


It might be better to refer to the implementation in DXC here, rather than copying the comment in DXC about compatibility with FXC.

I revised the comment to mention that the scaling factor is the same as FXC and also the DXC DXIL implementation.
I also kept the link to the stackoverflow discussion, as the comments in the DXC DXIL implementation referred to it but did not provide a link, which I think is kind of important.

Are these new comments more suitable? Or should I just simply just provide a link to the DXC implementation and cut out the repetition?

github-actions · 2025-01-14T17:12:26Z

@Icohedron Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics HLSL HLSL Language Support labels Jan 9, 2025

Icohedron commented Jan 9, 2025

View reviewed changes

Icohedron force-pushed the D3DCOLORtoUBYTE4-impl branch from 5159990 to a4bbeda Compare January 9, 2025 19:19

inbelic self-requested a review January 9, 2025 22:00

Icohedron force-pushed the D3DCOLORtoUBYTE4-impl branch from a4bbeda to 25bf1be Compare January 9, 2025 23:46

[HLSL] Implement the D3DCOLORtoUBYTE4 intrinsic

5610b22

Icohedron force-pushed the D3DCOLORtoUBYTE4-impl branch from 25bf1be to 5610b22 Compare January 10, 2025 00:01

inbelic approved these changes Jan 10, 2025

View reviewed changes

s-perron approved these changes Jan 10, 2025

View reviewed changes

Icohedron added 2 commits January 10, 2025 18:50

Address some PR comments

e2c4abd

Merge remote-tracking branch 'upstream/main' into feature-D3DCOLORtoU…

57f9048

…BYTE4

bogner reviewed Jan 13, 2025

View reviewed changes

Link to DXC implementation in comment

6d17064

farzonl approved these changes Jan 13, 2025

View reviewed changes

llvm-beanz approved these changes Jan 14, 2025

View reviewed changes

inbelic merged commit 06c6bae into llvm:main Jan 14, 2025
8 checks passed

Icohedron deleted the D3DCOLORtoUBYTE4-impl branch January 14, 2025 23:22

Icohedron mentioned this pull request Jan 21, 2025

Request Commit Access For Icohedron #123838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HLSL] Implement D3DCOLORtoUBYTE4 intrinsic #122202

[HLSL] Implement D3DCOLORtoUBYTE4 intrinsic #122202

Icohedron commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025

llvmbot commented Jan 9, 2025 •

edited

Loading

Icohedron Jan 9, 2025

s-perron Jan 10, 2025

inbelic left a comment

inbelic Jan 10, 2025

inbelic Jan 10, 2025

inbelic Jan 10, 2025

inbelic Jan 10, 2025

s-perron Jan 10, 2025

bogner Jan 13, 2025

Icohedron Jan 13, 2025

github-actions bot commented Jan 14, 2025

	constexpr vector<uint, 4> d3d_color_to_ubyte4(vector<float, 4> V) {
	constexpr vector<uint, 4> D3DColorToUByte4Impl(vector<float, 4> V) {

		@@ -0,0 +1,12 @@
		// RUN: %clang_cc1 -finclude-default-header -triple \
		// RUN: dxil-pc-shadermodel6.3-library %s -fnative-half-type \

	// CHECK: %[[SCALED:.]] = fmul [[FMFLAGS:.]]<4 x float> %{{.*}}, splat (float 0x406FE01000000000)
	// CHECK: %[[SCALED:.]] = fmul [[FMFLAGS:.]] [[FLOAT_TYPE:<4 x float>]] %{{.*}}, splat (float 0x406FE01000000000)

[HLSL] Implement D3DCOLORtoUBYTE4 intrinsic #122202

[HLSL] Implement D3DCOLORtoUBYTE4 intrinsic #122202

Conversation

Icohedron commented Jan 9, 2025 • edited Loading

github-actions bot commented Jan 9, 2025

llvmbot commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inbelic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2025

Icohedron commented Jan 9, 2025 •

edited

Loading

llvmbot commented Jan 9, 2025 •

edited

Loading