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

farzonl · 2025-04-18T01:01:31Z

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

Write a custom DiagnosticInfoUnsupported so we can change the Message just for DirectX. Too heavy handed so rejected.
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.
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.
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.
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 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.

llvmbot · 2025-04-18T01:02:05Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-directx

Author: Farzon Lotfi (farzonl)

Changes

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

Write a custom DiagnosticInfoUnsupported so we can change the Message just for DirectX. Too heavy handed so rejected.
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.
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.
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.
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.

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

3 Files Affected:

(added) clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl (+7)
(modified) llvm/lib/Target/DirectX/DXILOpLowering.cpp (+6-3)
(modified) llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll (+1-1)

diff --git a/clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl b/clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl
new file mode 100644
index 0000000000000..b10fb5d409f91
--- /dev/null
+++ b/clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl
@@ -0,0 +1,7 @@
+// RUN: %if clang-dxc %{not %clang_dxc -T lib_6_3 %s 2>&1 | FileCheck %s %}
+
+// CHECK: error: Unsupported intrinsic llvm.vector.reduce.and.v4i32 for DXIL lowering
+
+export int vecReduceAndTest(int4 vec) {
+    return __builtin_reduce_and(vec);
+}
diff --git a/llvm/lib/Target/DirectX/DXILOpLowering.cpp b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
index 4574e5f7bbd96..3ba963a5b4f8a 100644
--- a/llvm/lib/Target/DirectX/DXILOpLowering.cpp
+++ b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
@@ -8,7 +8,6 @@
 
 #include "DXILOpLowering.h"
 #include "DXILConstants.h"
-#include "DXILIntrinsicExpansion.h"
 #include "DXILOpBuilder.h"
 #include "DXILShaderFlags.h"
 #include "DirectX.h"
@@ -27,6 +26,7 @@
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 
 #define DEBUG_TYPE "dxil-op-lower"
 
@@ -756,8 +756,11 @@ class OpLowerer {
       case Intrinsic::not_intrinsic:
         continue;
       default: {
-        DiagnosticInfoUnsupported Diag(
-            F, "Unsupported intrinsic for DXIL lowering");
+        std::string Msg =
+            llvm::formatv("Unsupported intrinsic {0} for DXIL lowering",
+                          F.getName())
+                .str();
+        DiagnosticInfoUnsupported Diag(F, Msg);
         M.getContext().diagnose(Diag);
         HasErrors |= true;
         break;
diff --git a/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll b/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll
index d703f2f04c494..3ee5ff89b02a2 100644
--- a/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll
+++ b/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll
@@ -1,6 +1,6 @@
 ; RUN: not opt -S -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>): Unsupported intrinsic for DXIL lowering
+; CHECK: Unsupported intrinsic llvm.vector.reduce.and.v4i32 for DXIL lowering
 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

llvmbot · 2025-04-18T01:02:06Z

@llvm/pr-subscribers-clang

Author: Farzon Lotfi (farzonl)

Changes

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

Write a custom DiagnosticInfoUnsupported so we can change the Message just for DirectX. Too heavy handed so rejected.
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.
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.
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.
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.

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

3 Files Affected:

(added) clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl (+7)
(modified) llvm/lib/Target/DirectX/DXILOpLowering.cpp (+6-3)
(modified) llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll (+1-1)

diff --git a/clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl b/clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl
new file mode 100644
index 0000000000000..b10fb5d409f91
--- /dev/null
+++ b/clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl
@@ -0,0 +1,7 @@
+// RUN: %if clang-dxc %{not %clang_dxc -T lib_6_3 %s 2>&1 | FileCheck %s %}
+
+// CHECK: error: Unsupported intrinsic llvm.vector.reduce.and.v4i32 for DXIL lowering
+
+export int vecReduceAndTest(int4 vec) {
+    return __builtin_reduce_and(vec);
+}
diff --git a/llvm/lib/Target/DirectX/DXILOpLowering.cpp b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
index 4574e5f7bbd96..3ba963a5b4f8a 100644
--- a/llvm/lib/Target/DirectX/DXILOpLowering.cpp
+++ b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
@@ -8,7 +8,6 @@
 
 #include "DXILOpLowering.h"
 #include "DXILConstants.h"
-#include "DXILIntrinsicExpansion.h"
 #include "DXILOpBuilder.h"
 #include "DXILShaderFlags.h"
 #include "DirectX.h"
@@ -27,6 +26,7 @@
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 
 #define DEBUG_TYPE "dxil-op-lower"
 
@@ -756,8 +756,11 @@ class OpLowerer {
       case Intrinsic::not_intrinsic:
         continue;
       default: {
-        DiagnosticInfoUnsupported Diag(
-            F, "Unsupported intrinsic for DXIL lowering");
+        std::string Msg =
+            llvm::formatv("Unsupported intrinsic {0} for DXIL lowering",
+                          F.getName())
+                .str();
+        DiagnosticInfoUnsupported Diag(F, Msg);
         M.getContext().diagnose(Diag);
         HasErrors |= true;
         break;
diff --git a/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll b/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll
index d703f2f04c494..3ee5ff89b02a2 100644
--- a/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll
+++ b/llvm/test/CodeGen/DirectX/unsupported_intrinsic.ll
@@ -1,6 +1,6 @@
 ; RUN: not opt -S -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>): Unsupported intrinsic for DXIL lowering
+; CHECK: Unsupported intrinsic llvm.vector.reduce.and.v4i32 for DXIL lowering
 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

bogner · 2025-04-18T17:04:52Z

This doesn't seem right. Are we just misusing DiagnosticInfoUnsupported here? The Function argument for DiagnosticInfoWithLocationBase is clearly meant to be part of the location (as evidenced by the "in function XYZ" part of the message). I suspect that we should really be changing the unknown intrinsic diagnostic to use DiagnosticInfoGeneric rather than DiagnosticInfoUnsupported, and we should probably update the other use of DiagnosticInfoUnsupported in OpLowerer::replaceFunction to use DiagnosticInfoGenericWithLoc and pass in the parent function of the CallInst rather than the callee.

farzonl · 2025-04-18T17:12:33Z

This doesn't seem right. Are we just misusing DiagnosticInfoUnsupported here? The Function argument for DiagnosticInfoWithLocationBase is clearly meant to be part of the location (as evidenced by the "in function XYZ" part of the message). I suspect that we should really be changing the unknown intrinsic diagnostic to use DiagnosticInfoGeneric rather than DiagnosticInfoUnsupported, and we should probably update the other use of DiagnosticInfoUnsupported in OpLowerer::replaceFunction to use DiagnosticInfoGenericWithLoc and pass in the parent function of the CallInst rather than the callee.

Thats option 5. we would still want to encode the intrinsic name in the message but if we used DiagnosticInfoGeneric the message string and the print would be the same.

bogner · 2025-04-18T21:17:32Z

llvm/lib/Target/DirectX/DXILOpLowering.cpp

+        std::string Msg =
+            formatv("Unsupported intrinsic {0} for DXIL lowering", F.getName())
+                .str();


Might as well use SmallString here:

Suggested change

std::string Msg =

formatv("Unsupported intrinsic {0} for DXIL lowering", F.getName())

.str();

SmallString<128> Msg =

formatv("Unsupported intrinsic {0} for DXIL lowering", F.getName());

bogner · 2025-04-18T21:30:16Z

llvm/include/llvm/IR/DiagnosticInfo.h

-  const Twine &MsgStr;
+  const Twine MsgStr;


All of the Twine usage in Diagnostic info is super sketchy. I think we can drop this from this change and do it more comprehensively here: #136371

bogner · 2025-04-18T21:35:30Z

clang/test/CodeGenDirectX/unsupported_intrinsic.hlsl

@@ -0,0 +1,7 @@
+// RUN: %if clang-dxc %{not %clang_dxc -T lib_6_3 %s 2>&1 | FileCheck %s %}


I don't think there's a clang-dxc feature, so I suspect this just never runs at all. This will fail if the DirectX backend isn't available, so I don't think gating on clang-dxc would be correct anyway.

I think you actually want REQUIRES: directx-registered-target in this test, and to drop the %if stuff.

…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 Apr 18, 2025

llvmbot added clang Clang issues not falling into any other category backend:DirectX labels Apr 18, 2025

github-project-automation bot added this to HLSL Support Apr 18, 2025

farzonl added 2 commits April 18, 2025 10:26

address pr comments

e257a37

fix string lifetime issue

3217f06

llvmbot added the llvm:ir label Apr 18, 2025

bogner reviewed Apr 18, 2025

View reviewed changes

address pr feedback

49389f5

bogner approved these changes Apr 19, 2025

View reviewed changes

llvm-beanz approved these changes Apr 21, 2025

View reviewed changes

farzonl merged commit 111af76 into llvm:main Apr 21, 2025
12 checks passed

damyanp moved this to Closed in HLSL Support Apr 25, 2025

damyanp removed this from HLSL Support Jun 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

farzonl commented Apr 18, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 18, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

bogner commented Apr 18, 2025

Uh oh!

farzonl commented Apr 18, 2025

Uh oh!

bogner Apr 18, 2025

Uh oh!

bogner Apr 18, 2025

Uh oh!

bogner Apr 18, 2025

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,7 @@
		// RUN: %if clang-dxc %{not %clang_dxc -T lib_6_3 %s 2>&1 \| FileCheck %s %}

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

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

Uh oh!

Conversation

farzonl commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

bogner commented Apr 18, 2025

Uh oh!

farzonl commented Apr 18, 2025

Uh oh!

bogner Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

bogner Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

bogner Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

farzonl commented Apr 18, 2025 •

edited

Loading

llvmbot commented Apr 18, 2025 •

edited

Loading