[HLSL] Reapply Move length support out of the DirectX Backend (#121611) #122337

farzonl · 2025-01-09T18:58:40Z

attribute keeps getting deleted on bad rebases

llvmbot · 2025-01-09T18:59:13Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-backend-directx
@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-backend-x86

Author: Farzon Lotfi (farzonl)

Changes

attribute keeps getting deleted on bad rebases

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

1 Files Affected:

(modified) clang/lib/Headers/hlsl/hlsl_intrinsics.h (+2)

diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index cf287e598f76ba..7105d83078a9d7 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -1297,9 +1297,11 @@ float4 lerp(float4, float4, float4);
 ///
 /// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + ...).
 
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
 const inline half length(half X) { return __detail::length_impl(X); }
 const inline float length(float X) { return __detail::length_impl(X); }
 
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
 template <int N> const inline half length(vector<half, N> X) {
   return __detail::length_vec_impl(X);
 }

llvmbot · 2025-01-09T18:59:13Z

@llvm/pr-subscribers-clang

Author: Farzon Lotfi (farzonl)

Changes

attribute keeps getting deleted on bad rebases

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

1 Files Affected:

(modified) clang/lib/Headers/hlsl/hlsl_intrinsics.h (+2)

diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index cf287e598f76ba..7105d83078a9d7 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -1297,9 +1297,11 @@ float4 lerp(float4, float4, float4);
 ///
 /// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + ...).
 
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
 const inline half length(half X) { return __detail::length_impl(X); }
 const inline float length(float X) { return __detail::length_impl(X); }
 
+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
 template <int N> const inline half length(vector<half, N> X) {
   return __detail::length_vec_impl(X);
 }

damyanp

Does this fix a test failure?

inbelic

LGTM, assuming we don't need the other availability denoted

inbelic · 2025-01-09T19:06:44Z

clang/lib/Headers/hlsl/hlsl_intrinsics.h

@@ -1297,9 +1297,11 @@ float4 lerp(float4, float4, float4);
 ///
 /// Length is based on the following formula: sqrt(x[0]^2 + x[1]^2 + ...).

+_HLSL_16BIT_AVAILABILITY(shadermodel, 6.2)
 const inline half length(half X) { return __detail::length_impl(X); }
 const inline float length(float X) { return __detail::length_impl(X); }


Do we need _HLSL_AVAILABILITY(shadermodel, 6.2) on the none half variants?

farzonl · 2025-01-09T19:12:55Z

Does this fix a test failure?

No failures. We would need to have tests for all intrinsics that if its not SM 6.2 or greater uses of half would error, but we never did that for any of the other intrinsics so we should probably discuss what the test coverage should be and apply it broadly.

## Changes - Delete DirectX length intrinsic - Delete HLSL length lang builtin - Implement length algorithm entirely in the header. ## History - In the past if an HLSL intrinsic lowered to either a spirv op code or a DXIL opcode we represented it with intrinsics ## Why we are moving away? - To make HLSL apis more portable the team decided that it makes sense for some intrinsics to be defined only in the header. - Since there tends to be more SPIRV opcodes than DXIL opcodes the plan is to support SPIRV opcodes either with target specific builtins or via pattern matching.

- attribute keeps getting deleted on bad rebases

github-actions · 2025-01-09T22:00:34Z

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

llvm-beanz · 2025-01-09T23:29:20Z

No failures. We would need to have tests for all intrinsics that if its not SM 6.2 or greater uses of half would error, but we never did that for any of the other intrinsics so we should probably discuss what the test coverage should be and apply it broadly.

In practice these are pretty tricky to test on just Clang because the 6.2 attribute only applies when -enable-16bit-types is set, which requires shader model 6.2 or greater. So you shouldn't actually be able to make the compiler fail this.

The point of the attribute is really just for header completeness so that we can drive documentation and LSP tooling from it.

farzonl · 2025-01-10T00:45:07Z

@llvm-beanz I'm really confused what is going on here
The length test case passes in the github action: https://github.com/llvm/llvm-project/actions/runs/12699150463?pr=122337
It passes locally on:

MacOS Intel (Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz)
MacOS Apple M3 Max 12 core
Ubuntu 22.04 AMD Ryzen Threadripper PRO 5975WX 32-Cores
Windows AMD Ryzen 9 5900X

BUT its failing in buildkite and on other llvm builders. I'm not sure what could do this.
https://buildkite.com/llvm-project/github-pull-requests/builds/135222

farzonl · 2025-01-10T17:29:25Z

@llvm-beanz I'm really confused what is going on here The length test case passes in the github action: https://github.com/llvm/llvm-project/actions/runs/12699150463?pr=122337 It passes locally on:

MacOS Intel (Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz)

MacOS Apple M3 Max 12 core

Ubuntu 22.04 AMD Ryzen Threadripper PRO 5975WX 32-Cores

Windows AMD Ryzen 9 5900X

BUT its failing in buildkite and on other llvm builders. I'm not sure what could do this. https://buildkite.com/llvm-project/github-pull-requests/builds/135222

I figured out what is going on here. On some platforms the loop unroll isn't working

This warning shows up:

warning: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering

And there is a br label %for.body.i.

I'm switching to define length as sqrt(dot(x,x)) to avoid the loop.

…to fix test break.

farzonl · 2025-01-10T19:15:32Z

Ok going to try to merge again. no more test failure for length.hlsl in buildkite. only failing test was: Clang.SemaSYCL/sycl-kernel-entry-point-attr-appertainment.cpp

llvm-ci · 2025-01-10T22:39:02Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-expensive-checks-debian running on gribozavr4 while building clang,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/11794

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: tools/llvm-gsymutil/ARM_AArch64/macho-merged-funcs-dwarf.yaml' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
Input file: /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-gsymutil/ARM_AArch64/Output/macho-merged-funcs-dwarf.yaml.tmp.dSYM
Output file (aarch64): /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-gsymutil/ARM_AArch64/Output/macho-merged-funcs-dwarf.yaml.tmp.default.gSYM
Loaded 3 functions from DWARF.
Loaded 3 functions from symbol table.
warning: same address range contains different debug info. Removing:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000047
addr=0x0000000000000248, file=  3, line=  5
addr=0x0000000000000254, file=  3, line=  7
addr=0x0000000000000258, file=  3, line=  9
addr=0x000000000000025c, file=  3, line=  8
addr=0x0000000000000260, file=  3, line= 11
addr=0x0000000000000264, file=  3, line= 10
addr=0x0000000000000268, file=  3, line=  6


In favor of this one:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000030
addr=0x0000000000000248, file=  2, line=  5
addr=0x0000000000000254, file=  2, line=  7
addr=0x0000000000000258, file=  2, line=  9
addr=0x000000000000025c, file=  2, line=  8
addr=0x0000000000000260, file=  2, line= 11
addr=0x0000000000000264, file=  2, line= 10
addr=0x0000000000000268, file=  2, line=  6


warning: same address range contains different debug info. Removing:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000030
addr=0x0000000000000248, file=  2, line=  5
addr=0x0000000000000254, file=  2, line=  7
addr=0x0000000000000258, file=  2, line=  9
addr=0x000000000000025c, file=  2, line=  8
addr=0x0000000000000260, file=  2, line= 11
addr=0x0000000000000264, file=  2, line= 10
addr=0x0000000000000268, file=  2, line=  6


In favor of this one:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000001
addr=0x0000000000000248, file=  1, line=  5
addr=0x0000000000000254, file=  1, line=  7
addr=0x0000000000000258, file=  1, line=  9
addr=0x000000000000025c, file=  1, line=  8
addr=0x0000000000000260, file=  1, line= 11
addr=0x0000000000000264, file=  1, line= 10
...

…21611) (llvm#122337) ## Changes - Delete DirectX length intrinsic - Delete HLSL length lang builtin - Implement length algorithm entirely in the header. ## History - In the past if an HLSL intrinsic lowered to either a spirv op code or a DXIL opcode we represented it with intrinsics ## Why we are moving away? - To make HLSL apis more portable the team decided that it makes sense for some intrinsics to be defined only in the header. - Since there tends to be more SPIRV opcodes than DXIL opcodes the plan is to support SPIRV opcodes either with target specific builtins or via pattern matching.

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

damyanp approved these changes Jan 9, 2025

View reviewed changes

inbelic approved these changes Jan 9, 2025

View reviewed changes

farzonl force-pushed the fix-length-half-availability branch from 0f0b75d to 6f2da80 Compare January 9, 2025 21:24

llvmbot added clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. backend:DirectX HLSL HLSL Language Support llvm:ir labels Jan 9, 2025

farzonl changed the title ~~[HLSL] Add 6.2 SM on half availability for length intrinsic~~ [HLSL] Reapply Move length support out of the DirectX Backend (llvm#121611) Jan 9, 2025

farzonl changed the title ~~[HLSL] Reapply Move length support out of the DirectX Backend (llvm#121611)~~ [HLSL] Reapply Move length support out of the DirectX Backend (#121611) Jan 9, 2025

farzonl mentioned this pull request Jan 9, 2025

[HLSL] Move length support out of the DirectX Backend #121611

Merged

[HLSL] Add 6.2 SM on half availability for length intrinsic

7173a48

- attribute keeps getting deleted on bad rebases

farzonl force-pushed the fix-length-half-availability branch from 6f2da80 to 7173a48 Compare January 9, 2025 21:57

llvm-beanz approved these changes Jan 9, 2025

View reviewed changes

replace loop unroll length algo with one that doesn't have to unroll …

15620e5

…to fix test break.

farzonl merged commit b900379 into llvm:main Jan 10, 2025
6 of 8 checks passed

farzonl deleted the fix-length-half-availability branch January 13, 2025 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HLSL] Reapply Move length support out of the DirectX Backend (#121611) #122337

[HLSL] Reapply Move length support out of the DirectX Backend (#121611) #122337

Uh oh!

farzonl commented Jan 9, 2025

Uh oh!

llvmbot commented Jan 9, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 9, 2025

Uh oh!

damyanp left a comment

Uh oh!

inbelic left a comment

Uh oh!

inbelic Jan 9, 2025

Uh oh!

farzonl commented Jan 9, 2025

Uh oh!

github-actions bot commented Jan 9, 2025 •

edited

Loading

Uh oh!

llvm-beanz commented Jan 9, 2025

Uh oh!

farzonl commented Jan 10, 2025

Uh oh!

farzonl commented Jan 10, 2025

Uh oh!

farzonl commented Jan 10, 2025

Uh oh!

Uh oh!

llvm-ci commented Jan 10, 2025

Uh oh!

Uh oh!

[HLSL] Reapply Move length support out of the DirectX Backend (#121611) #122337

[HLSL] Reapply Move length support out of the DirectX Backend (#121611) #122337

Uh oh!

Conversation

farzonl commented Jan 9, 2025

Uh oh!

llvmbot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 9, 2025

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

inbelic left a comment

Choose a reason for hiding this comment

Uh oh!

inbelic Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

farzonl commented Jan 9, 2025

Uh oh!

github-actions bot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvm-beanz commented Jan 9, 2025

Uh oh!

farzonl commented Jan 10, 2025

Uh oh!

farzonl commented Jan 10, 2025

Uh oh!

farzonl commented Jan 10, 2025

Uh oh!

Uh oh!

llvm-ci commented Jan 10, 2025

Uh oh!

Uh oh!

llvmbot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading