Skip to content
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

[SPIRV] Add radians intrinsic #110800

Merged
merged 5 commits into from
Oct 4, 2024
Merged

[SPIRV] Add radians intrinsic #110800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
010cf37
Added radians selection for spirv
adam-yang Oct 2, 2024
ea6331e
Added nomem attribute
adam-yang Oct 2, 2024
735e8be
Fixed bad rebase. Removed selectRadians and using selectExtInst instead
adam-yang Oct 2, 2024
0ab8d0f
Added test variant for opencl
adam-yang Oct 3, 2024
17175f2
Separated the opencl test to its own thing
adam-yang Oct 3, 2024
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
1 change: 1 addition & 0 deletions llvm/include/llvm/IR/IntrinsicsSPIRV.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,5 @@ let TargetPrefix = "spv" in {
[IntrNoMem, Commutative] >;
def int_spv_wave_is_first_lane : DefaultAttrsIntrinsic<[llvm_i1_ty], [], [IntrConvergent]>;
def int_spv_sign : DefaultAttrsIntrinsic<[LLVMScalarOrSameVectorWidth<0, llvm_i32_ty>], [llvm_any_ty], [IntrNoMem]>;
def int_spv_radians : DefaultAttrsIntrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty], [IntrNoMem]>;
}
2 changes: 2 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2534,6 +2534,8 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
}
case Intrinsic::spv_step:
return selectExtInst(ResVReg, ResType, I, CL::step, GL::Step);
case Intrinsic::spv_radians:
return selectExtInst(ResVReg, ResType, I, CL::radians, GL::Radians);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test for CL::radians?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified the test to also test the CL variant.

Copy link
Member

@farzonl farzonl Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't use the opencl extension in hlsl though. Even though this is an easy test change llvm/test/CodeGen/SPIRV/hlsl-intrinsics tests should be devoted to how HLSL does things. I feel like the folks that care about opencl should be adding opencl tests to their parts of the test directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

CL variant could support double type and vector2/3/4/8/16.
It would be nice to have these tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use the opencl extension in hlsl though. Even though this is an easy test change llvm/test/CodeGen/SPIRV/hlsl-intrinsics tests should be devoted to how HLSL does things. I feel like the folks that care about opencl should be adding opencl tests to their parts of the test directory.

Can we have a common place to test things like int_spv_radians so that we can share the same test with different run lines, similar to what Adam did in radians.ll?

Copy link
Member

Choose a reason for hiding this comment

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

I’ll let other folks weigh in. I was afraid of testing scope creep. We do want a set of tests that are hlsl only. The question is do we want to be on the hook for opencl tests and do we want to go and update every hlsl intrinsic to have cl tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are adding the spv_radians -> cl::radians lowering code path, so we should test it.

Copy link
Member

Choose a reason for hiding this comment

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

Thats because selectExtInst defined in such a way where GL is optional but CL is required. Also I'm not against testing cl::radians. I just don't think it should be a part of the llvm/test/CodeGen/SPIRV/hlsl-intrinsics test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated the cl specific tests to llvm/test/CodeGen/SPIRV/opencl/radians.ll

Copy link
Member

Choose a reason for hiding this comment

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

I separated the cl specific tests to llvm/test/CodeGen/SPIRV/opencl/radians.ll

@adam-yang Thank you!

// Discard intrinsics which we do not expect to actually represent code after
// lowering or intrinsics which are not implemented but should not crash when
// found in a customer's LLVM IR input.
Expand Down
48 changes: 48 additions & 0 deletions llvm/test/CodeGen/SPIRV/hlsl-intrinsics/radians.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val %}

; CHECK-DAG: %[[#op_ext_glsl:]] = OpExtInstImport "GLSL.std.450"

; CHECK-DAG: %[[#float_32:]] = OpTypeFloat 32
; CHECK-DAG: %[[#float_16:]] = OpTypeFloat 16

; CHECK-DAG: %[[#vec4_float_32:]] = OpTypeVector %[[#float_32]] 4
; CHECK-DAG: %[[#vec4_float_16:]] = OpTypeVector %[[#float_16]] 4

declare half @llvm.spv.radians.f16(half)
declare float @llvm.spv.radians.f32(float)

declare <4 x float> @llvm.spv.radians.v4f32(<4 x float>)
declare <4 x half> @llvm.spv.radians.v4f16(<4 x half>)

define noundef float @radians_float(float noundef %a) {
entry:
; CHECK: %[[#float_32_arg:]] = OpFunctionParameter %[[#float_32]]
; CHECK: %[[#]] = OpExtInst %[[#float_32]] %[[#op_ext_glsl]] Radians %[[#float_32_arg]]
%elt.radians = call float @llvm.spv.radians.f32(float %a)
ret float %elt.radians
}

define noundef half @radians_half(half noundef %a) {
entry:
; CHECK: %[[#float_16_arg:]] = OpFunctionParameter %[[#float_16]]
; CHECK: %[[#]] = OpExtInst %[[#float_16]] %[[#op_ext_glsl]] Radians %[[#float_16_arg]]
%elt.radians = call half @llvm.spv.radians.f16(half %a)
ret half %elt.radians
}

define noundef <4 x float> @radians_float_vector(<4 x float> noundef %a) {
entry:
; CHECK: %[[#vec4_float_32_arg:]] = OpFunctionParameter %[[#vec4_float_32]]
; CHECK: %[[#]] = OpExtInst %[[#vec4_float_32]] %[[#op_ext_glsl]] Radians %[[#vec4_float_32_arg]]
%elt.radians = call <4 x float> @llvm.spv.radians.v4f32(<4 x float> %a)
ret <4 x float> %elt.radians
}

define noundef <4 x half> @radians_half_vector(<4 x half> noundef %a) {
entry:
; CHECK: %[[#vec4_float_16_arg:]] = OpFunctionParameter %[[#vec4_float_16]]
; CHECK: %[[#]] = OpExtInst %[[#vec4_float_16]] %[[#op_ext_glsl]] Radians %[[#vec4_float_16_arg]]
%elt.radians = call <4 x half> @llvm.spv.radians.v4f16(<4 x half> %a)
ret <4 x half> %elt.radians
}
51 changes: 51 additions & 0 deletions llvm/test/CodeGen/SPIRV/opencl/radians.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}

; CHECK-DAG: %[[#op_ext_glsl:]] = OpExtInstImport "OpenCL.std"

; CHECK-DAG: %[[#float_32:]] = OpTypeFloat 32
; CHECK-DAG: %[[#float_16:]] = OpTypeFloat 16

; CHECK-DAG: %[[#vec4_float_32:]] = OpTypeVector %[[#float_32]] 4
; CHECK-DAG: %[[#vec4_float_16:]] = OpTypeVector %[[#float_16]] 4

declare half @llvm.spv.radians.f16(half)
declare float @llvm.spv.radians.f32(float)

declare <4 x float> @llvm.spv.radians.v4f32(<4 x float>)
declare <4 x half> @llvm.spv.radians.v4f16(<4 x half>)

define noundef float @radians_float(float noundef %a) {
entry:
; CHECK: %[[#float_32_arg:]] = OpFunctionParameter %[[#float_32]]
; CHECK: %[[#]] = OpExtInst %[[#float_32]] %[[#op_ext_glsl]] radians %[[#float_32_arg]]
%elt.radians = call float @llvm.spv.radians.f32(float %a)
ret float %elt.radians
}

define noundef half @radians_half(half noundef %a) {
entry:
; CHECK: %[[#float_16_arg:]] = OpFunctionParameter %[[#float_16]]
; CHECK: %[[#]] = OpExtInst %[[#float_16]] %[[#op_ext_glsl]] radians %[[#float_16_arg]]
%elt.radians = call half @llvm.spv.radians.f16(half %a)
ret half %elt.radians
}

define noundef <4 x float> @radians_float_vector(<4 x float> noundef %a) {
entry:
; CHECK: %[[#vec4_float_32_arg:]] = OpFunctionParameter %[[#vec4_float_32]]
; CHECK: %[[#]] = OpExtInst %[[#vec4_float_32]] %[[#op_ext_glsl]] radians %[[#vec4_float_32_arg]]
%elt.radians = call <4 x float> @llvm.spv.radians.v4f32(<4 x float> %a)
ret <4 x float> %elt.radians
}

define noundef <4 x half> @radians_half_vector(<4 x half> noundef %a) {
entry:
; CHECK: %[[#vec4_float_16_arg:]] = OpFunctionParameter %[[#vec4_float_16]]
; CHECK: %[[#]] = OpExtInst %[[#vec4_float_16]] %[[#op_ext_glsl]] radians %[[#vec4_float_16_arg]]
%elt.radians = call <4 x half> @llvm.spv.radians.v4f16(<4 x half> %a)
ret <4 x half> %elt.radians
}

Loading