Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 25, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69f0295
[DirectX] only allow intrinsics defined in DXIL.td
farzonl Feb 25, 2025
3aacbd7
allow dx_resource_casthandle through and non intrinsic functions, fix…
farzonl Feb 25, 2025
f19dcf6
address pr comments
farzonl Feb 25, 2025
d40e8ff
Address PR comments, change diag string to indicate an intrinsic is n…
farzonl Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion llvm/lib/Target/DirectX/DXILOpLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,8 +770,14 @@ class OpLowerer {
continue;
Intrinsic::ID ID = F.getIntrinsicID();
switch (ID) {
default:
case Intrinsic::dx_resource_casthandle:
case Intrinsic::not_intrinsic:
continue;
default: {
DiagnosticInfoUnsupported Diag(F, "Unknown intrinsic?");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

M.getContext().diagnose(Diag);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

break;
}
#define DXIL_OP_INTRINSIC(OpCode, Intrin, ...) \
case Intrin: \
HasErrors |= replaceFunctionWithOp( \
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/CodeGen/DirectX/clamp.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ declare <3 x half> @llvm.dx.nclamp.v3f16(<3 x half>, <3 x half>, <3 x half>)
declare <4 x float> @llvm.dx.nclamp.v4f32(<4 x float>, <4 x float>, <4 x float>)
declare <2 x double> @llvm.dx.nclamp.v2f64(<2 x double>, <2 x double>, <2 x double>)
declare <4 x i32> @llvm.dx.sclamp.v4i32(<4 x i32>, <4 x i32>, <4 x i32>)
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>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@farzonl farzonl Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

declare <4 x i32> @llvm.dx.uclamp.v4i32(<4 x i32>, <4 x i32>, <4 x i32>)
declare <2 x i64> @llvm.dx.uclamp.v2i64(<2 x i64>, <2 x i64>, <2 x i64>)

2 changes: 1 addition & 1 deletion llvm/test/CodeGen/DirectX/discard.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
; RUN: opt -passes='function(scalarizer),module(dxil-op-lower,dxil-intrinsic-expansion)' -S -mtriple=dxil-pc-shadermodel6.3-pixel %s | FileCheck %s
; RUN: opt -passes='function(scalarizer),module(dxil-intrinsic-expansion,dxil-op-lower)' -S -mtriple=dxil-pc-shadermodel6.3-pixel %s | FileCheck %s

; CHECK-LABEL: define void @test_scalar
; CHECK: call void @dx.op.discard(i32 82, i1 %0)
Expand Down
11 changes: 11 additions & 0 deletions llvm/test/CodeGen/DirectX/unknown_intrinsic.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
; RUN: not opt -S -scalarizer -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s 2>&1 | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-scalarizer doesn't seem necessary here.


; 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"}
Loading