[DirectX] only allow intrinsics defined in DXIL.td #128613

farzonl · 2025-02-25T01:32:14Z

Fixes #128071
The current behavior lets intrinsics that don't map to a DXILOP slip through. Nothing catches this until we hit the DXIL validator. This change fails earlier so we don't encode invalid llvm intrinsics that can slip through because of clang builtins like __builtin_reduce_and
example:
https://hlsl.godbolt.org/z/13rPj18vn

Fixes llvm#128071 The current behavior lets intrinsics that don't map to a DXILOP slip through. Nothing catches this until we hit the DXIL validator. This change fails earlier so we don't encode invalid llvm intrinsics that can slip through because of clang builtins like `__builtin_reduce_and` example: https://hlsl.godbolt.org/z/13rPj18vn

llvmbot · 2025-02-25T01:32:48Z

@llvm/pr-subscribers-backend-directx

Author: Farzon Lotfi (farzonl)

Changes

Fixes #128071
The current behavior lets intrinsics that don't map to a DXILOP slip through. Nothing catches this until we hit the DXIL validator. This change fails earlier so we don't encode invalid llvm intrinsics that can slip through because of clang builtins like __builtin_reduce_and
example:
https://hlsl.godbolt.org/z/13rPj18vn

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

2 Files Affected:

(modified) llvm/lib/Target/DirectX/DXILOpLowering.cpp (+5-2)
(added) llvm/test/CodeGen/DirectX/unknown_intrinsic.ll (+11)

diff --git a/llvm/lib/Target/DirectX/DXILOpLowering.cpp b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
index 83cc4b18824c7..a98107a71c219 100644
--- a/llvm/lib/Target/DirectX/DXILOpLowering.cpp
+++ b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
@@ -770,8 +770,11 @@ class OpLowerer {
         continue;
       Intrinsic::ID ID = F.getIntrinsicID();
       switch (ID) {
-      default:
-        continue;
+      default: {
+        DiagnosticInfoUnsupported Diag(F, "Unknown intrinsic?");
+        M.getContext().diagnose(Diag);
+        break;
+      }
 #define DXIL_OP_INTRINSIC(OpCode, Intrin, ...)                                 \
   case Intrin:                                                                 \
     HasErrors |= replaceFunctionWithOp(                                        \
diff --git a/llvm/test/CodeGen/DirectX/unknown_intrinsic.ll b/llvm/test/CodeGen/DirectX/unknown_intrinsic.ll
new file mode 100644
index 0000000000000..efd0c5637077e
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/unknown_intrinsic.ll
@@ -0,0 +1,11 @@
+; RUN: not opt -S -scalarizer -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s 2>&1 | FileCheck %s
+
+; CHECK: error: <unknown>:0:0: in function llvm.vector.reduce.and.v4i32 i32 (<4 x i32>): Unknown intrinsic?
+define i32 @fn_and(<4 x i32> %0) local_unnamed_addr #0 {
+  %2 = tail call i32 @llvm.vector.reduce.and.v4i32(<4 x i32> %0)
+  ret i32 %2
+}
+
+declare i32 @llvm.vector.reduce.and.v4i32(<4 x i32>)
+
+attributes #0 = { convergent norecurse nounwind "hlsl.export"}

… up some testcases so passes happen in right order and intrinsics have the right declarations

farzonl · 2025-02-25T02:58:35Z

Need to investigate a tools/dxil-dis/debug-info.ll failure before merging.

llvm/lib/Target/DirectX/DXILOpLowering.cpp

bogner · 2025-02-25T03:34:22Z

llvm/lib/Target/DirectX/DXILOpLowering.cpp

        continue;
+      default: {
+        DiagnosticInfoUnsupported Diag(F, "Unknown intrinsic?");


We should probably include the name of the intrinsic in the error message here. It'd also be nice to have a debug location, but I suspect there isn't really anywhere we can grab a reasonable one from so I guess we'll do without.

The intrinsic name seems to already be encoded this is the error string i see function llvm.vector.reduce.and.v4i32 i32 (<4 x i32>): Unknown intrinsic?

Perhaps "Unsupported intrinsic for DXIL lowering", but this seems informative enough.

bogner · 2025-02-25T03:35:50Z

llvm/test/CodeGen/DirectX/unknown_intrinsic.ll

@@ -0,0 +1,11 @@
+; RUN: not opt -S -scalarizer -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s 2>&1 | FileCheck %s


-scalarizer doesn't seem necessary here.

bogner · 2025-02-25T03:37:08Z

llvm/lib/Target/DirectX/DXILOpLowering.cpp

        continue;
+      default: {
+        DiagnosticInfoUnsupported Diag(F, "Unknown intrinsic?");
+        M.getContext().diagnose(Diag);


Should we also set HasErrors here? It doesn't actually matter, since we won't leave the casthandles in an inconsistent state and we don't key anything else off of that, but it seems weird not to

I'll add it for consistency. We don't seem to be able to do multiple Diagnostic errors per test case. Ie I can only invoke M.getContext().diagnose(...) once. So setting HasErrors seemed pointless.

That's because you're using opt, which has the default error handler that just exits the program. If you want a test case with multiple errors you need to use llc (or clang, but that wouldn't be appropriate for this kind of test)

bogner · 2025-02-25T03:41:01Z

Need to investigate a tools/dxil-dis/debug-info.ll failure before merging.

I think we need to let llvm.dbg.value through. It's a bit annoying that we seem to be emitting bitcode when we have errors here, but that seems like an issue that's unrelated to this change.

bogner · 2025-02-25T09:54:59Z

llvm/lib/Target/DirectX/DXILOpLowering.cpp

        continue;
+      default: {
+        DiagnosticInfoUnsupported Diag(F, "Unknown intrinsic?");


Perhaps "Unsupported intrinsic for DXIL lowering", but this seems informative enough.

bogner · 2025-02-25T09:56:36Z

llvm/lib/Target/DirectX/DXILOpLowering.cpp

        continue;
+      default: {
+        DiagnosticInfoUnsupported Diag(F, "Unknown intrinsic?");
+        M.getContext().diagnose(Diag);


That's because you're using opt, which has the default error handler that just exits the program. If you want a test case with multiple errors you need to use llc (or clang, but that wouldn't be appropriate for this kind of test)

…ot supported

damyanp · 2025-02-25T20:35:45Z

llvm/test/CodeGen/DirectX/clamp.ll

-declare <3 x i16> @llvm.dx.uclamp.v3i32(<3 x i16>, <3 x i32>, <3 x i16>)
+declare <3 x i16> @llvm.dx.uclamp.v3i16(<3 x i16>, <3 x i16>, <3 x i16>)


Why did this change?

It's just to make the declaration match the function that is actually used.
I think this is an NFC. We could remove the declarations and the test would still be fine.
I don't see harm in including this change.

Well one its a typo. but it broke when I changed to error by default.There was no uclamp.v3i32 usage only a declare. There was a uclamp.v3i16 usage but no declare.

If we had a use then after the DXILIntrinsicExpansion pass ran it would have done cleanup for the decalres aswell. As is though this intrinsic declaration lives on and triggered our error because there is n uclamp lowering to a dxil op.

Icohedron

Looks ok to me

fixes llvm#135654 In llvm#128613 we added safe guards to prevent the lowering of just any intrinsic in the backend. We used `DiagnosticInfoUnsupported` to do this. What we found was when using `opt` the diagnostic printer was called but when using clang the diagnostic message was called. Printing message in the clang version means we miss valuable debugging information like function name and function type when LLVMContext was only needed to call `getBestLocationFromDebugLoc`.

…pLowering fixes llvm#135654 In llvm#128613 we added safe guards to prevent the lowering of just any intrinsic in the backend. We used `DiagnosticInfoUnsupported` to do this. What we found was when using `opt` the diagnostic print function was called but when using clang the diagnostic message was used. Printing message in the clang version means we miss valuable debugging information like function name and function type when LLVMContext was only needed to call `getBestLocationFromDebugLoc`. There are a few potential fixes 1. Write a custom DiagnosticInfoUnsupported so we can change the Message just for DirectX. Too heavy handed so rejected. 2. Add the function name to the Message in DirectX code. Very simple one line change. Downside is when using opt you see the function name twice. But makes the clang-dxc bugs more actionable. 3. change CodeGenAction.cpp to always use the print function and not the message directly. Downside is a bunch of innacurate information shows up in the message if you don't specify `-debug-info-kind=standalone`. 4. add some book keeping to know which function called the intrinsic keep a map of these so we can pass the calling function to `DiagnosticInfoUnsupported` instead of the intrinsic. This would only be useful if we had debug info so we could distinguish different uses of the intrinsic. We would also need to change from iterating on every function to doing somethin like a LazyCallGraph. 5. pick a different means of doing a Diagnostic error, because other uses of`DiagnosticInfoUnsupported` error when we are in the body of a function not when we see one being used like in the intrinsic case. This PR went with option 2. Its low code change that also only impacts the DirectX backend.

…pLowering (#136234) fixes #135654 In #128613 we added safe guards to prevent the lowering of just any intrinsic in the backend. We used `DiagnosticInfoUnsupported` to do this. What we found was when using `opt` the diagnostic print function was called but when using clang the diagnostic message was used. Printing message in the clang version means we miss valuable debugging information like function name and function type when LLVMContext was only needed to call `getBestLocationFromDebugLoc`. There are a few potential fixes 1. Write a custom DiagnosticInfoUnsupported so we can change the Message just for DirectX. Too heavy handed so rejected. 2. Add the function name to the Message in DirectX code. Very simple one line change. Downside is when using opt you see the function name twice. But makes the clang-dxc bugs more actionable. 3. change CodeGenAction.cpp to always use the print function and not the message directly. Downside is a bunch of innacurate information shows up in the message if you don't specify `-debug-info-kind=standalone`. 4. add some book keeping to know which function called the intrinsic keep a map of these so we can pass the calling function to `DiagnosticInfoUnsupported` instead of the intrinsic. This would only be useful if we had debug info so we could distinguish different uses of the intrinsic by line\col number. We would also need to change from iterating on every function to doing something like a LazyCallGraph which is a nonstarter. 5. pick a different means of doing a Diagnostic error, because other uses of `DiagnosticInfoUnsupported` error when we are in the body of a function not when we see one being used like in the intrinsic case. This PR went with a combo of option 2 & 5. Its low code change that also only impacts the DirectX backend.

…pLowering (llvm#136234) fixes llvm#135654 In llvm#128613 we added safe guards to prevent the lowering of just any intrinsic in the backend. We used `DiagnosticInfoUnsupported` to do this. What we found was when using `opt` the diagnostic print function was called but when using clang the diagnostic message was used. Printing message in the clang version means we miss valuable debugging information like function name and function type when LLVMContext was only needed to call `getBestLocationFromDebugLoc`. There are a few potential fixes 1. Write a custom DiagnosticInfoUnsupported so we can change the Message just for DirectX. Too heavy handed so rejected. 2. Add the function name to the Message in DirectX code. Very simple one line change. Downside is when using opt you see the function name twice. But makes the clang-dxc bugs more actionable. 3. change CodeGenAction.cpp to always use the print function and not the message directly. Downside is a bunch of innacurate information shows up in the message if you don't specify `-debug-info-kind=standalone`. 4. add some book keeping to know which function called the intrinsic keep a map of these so we can pass the calling function to `DiagnosticInfoUnsupported` instead of the intrinsic. This would only be useful if we had debug info so we could distinguish different uses of the intrinsic by line\col number. We would also need to change from iterating on every function to doing something like a LazyCallGraph which is a nonstarter. 5. pick a different means of doing a Diagnostic error, because other uses of `DiagnosticInfoUnsupported` error when we are in the body of a function not when we see one being used like in the intrinsic case. This PR went with a combo of option 2 & 5. Its low code change that also only impacts the DirectX backend.

farzonl self-assigned this Feb 25, 2025

llvmbot added the backend:DirectX label Feb 25, 2025

farzonl marked this pull request as draft February 25, 2025 01:39

allow dx_resource_casthandle through and non intrinsic functions, fix…

3aacbd7

… up some testcases so passes happen in right order and intrinsics have the right declarations

farzonl force-pushed the allow-only-legal-intrinsics branch from bfd8361 to 3aacbd7 Compare February 25, 2025 02:27

farzonl marked this pull request as ready for review February 25, 2025 02:27

bogner approved these changes Feb 25, 2025

View reviewed changes

address pr comments

f19dcf6

bogner approved these changes Feb 25, 2025

View reviewed changes

Address PR comments, change diag string to indicate an intrinsic is n…

d40e8ff

…ot supported

damyanp reviewed Feb 25, 2025

View reviewed changes

Icohedron approved these changes Feb 25, 2025

View reviewed changes

farzonl merged commit 0be3f13 into llvm:main Feb 25, 2025
12 checks passed

farzonl mentioned this pull request Apr 14, 2025

[Clang][DirectX] Always use Diagnostic Printer #135655

Closed

farzonl mentioned this pull request Apr 18, 2025

[DirectX] add Function name to DiagnosticInfoUnsupported Msg in DXILOpLowering #136234

Merged

damyanp removed this from HLSL Support Jun 25, 2025

		@@ -0,0 +1,11 @@
		; RUN: not opt -S -scalarizer -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s 2>&1 \| FileCheck %s

		declare <3 x i16> @llvm.dx.uclamp.v3i32(<3 x i16>, <3 x i32>, <3 x i16>)
		declare <3 x i16> @llvm.dx.uclamp.v3i16(<3 x i16>, <3 x i16>, <3 x i16>)

[DirectX] only allow intrinsics defined in DXIL.td #128613

[DirectX] only allow intrinsics defined in DXIL.td #128613

Uh oh!

Conversation

farzonl commented Feb 25, 2025

Uh oh!

llvmbot commented Feb 25, 2025

Uh oh!

farzonl commented Feb 25, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogner commented Feb 25, 2025

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!

farzonl Feb 25, 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!

Icohedron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

farzonl Feb 25, 2025 •

edited

Loading