[SPIRV] Add radians intrinsic #110800

adam-yang · 2024-10-02T08:29:14Z

partially fixes #99151

Changes

Added int_spv_radians intrinsic in IntrinsicsSPIRV.td
Added lowering for int_spv_radians in SPIRVInstructionSelector.cpp
Added DXIL backend test case

Related PRs

github-actions · 2024-10-02T08:29:33Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-10-02T08:51:40Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-spir-v

Author: Adam Yang (adam-yang)

Changes

partially fixes #99151

Changes

Added int_spv_radians intrinsic in IntrinsicsSPIRV.td
Added lowering for int_spv_radians in SPIRVInstructionSelector.cpp
Added DXIL backend test case

Related PRs

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

3 Files Affected:

(modified) llvm/include/llvm/IR/IntrinsicsSPIRV.td (+1)
(modified) llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp (+22)
(added) llvm/test/CodeGen/SPIRV/hlsl-intrinsics/radians.ll (+48)

diff --git a/llvm/include/llvm/IR/IntrinsicsSPIRV.td b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
index 7ff3d58690ba75..c541f99942bd9c 100644
--- a/llvm/include/llvm/IR/IntrinsicsSPIRV.td
+++ b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
@@ -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]>;
+  def int_spv_radians : DefaultAttrsIntrinsic<[LLVMMatchType<0>], [llvm_anyfloat_ty], [IntrNoMem]>;
 }
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 2f7efbdc81f845..3a786eb879b39f 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -247,6 +247,9 @@ class SPIRVInstructionSelector : public InstructionSelector {
   bool selectStep(Register ResVReg, const SPIRVType *ResType,
                   MachineInstr &I) const;
 
+  bool selectRadians(Register ResVReg, const SPIRVType *ResType,
+                     MachineInstr &I) const;
+
   bool selectUnmergeValues(MachineInstr &I) const;
 
   Register buildI32Constant(uint32_t Val, MachineInstr &I,
@@ -1783,6 +1786,23 @@ bool SPIRVInstructionSelector::selectStep(Register ResVReg,
       .constrainAllUses(TII, TRI, RBI);
 }
 
+bool SPIRVInstructionSelector::selectRadians(Register ResVReg,
+                                          const SPIRVType *ResType,
+                                          MachineInstr &I) const {
+
+  assert(I.getNumOperands() == 3);
+  assert(I.getOperand(2).isReg());
+  MachineBasicBlock &BB = *I.getParent();
+
+  return BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpExtInst))
+      .addDef(ResVReg)
+      .addUse(GR.getSPIRVTypeID(ResType))
+      .addImm(static_cast<uint32_t>(SPIRV::InstructionSet::GLSL_std_450))
+      .addImm(GL::Radians)
+      .addUse(I.getOperand(2).getReg())
+      .constrainAllUses(TII, TRI, RBI);
+}
+
 bool SPIRVInstructionSelector::selectBitreverse(Register ResVReg,
                                                 const SPIRVType *ResType,
                                                 MachineInstr &I) const {
@@ -2556,6 +2576,8 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
   }
   case Intrinsic::spv_step:
     return selectStep(ResVReg, ResType, I);
+  case Intrinsic::spv_radians:
+    return selectRadians(ResVReg, ResType, I);
   case Intrinsic::spv_value_md:
     // ignore the intrinsic
     break;
diff --git a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/radians.ll b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/radians.ll
new file mode 100644
index 00000000000000..1fe8ab30ed9538
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/radians.ll
@@ -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
+}

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

farzonl

You need to rebase, you shouldn't need a custom select for radians because it's spec'd out in GLSL extensions: https://registry.khronos.org/SPIR-V/specs/1.0/GLSL.std.450.html

python3kgae · 2024-10-03T00:18:29Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

@@ -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);


Do we need a test for CL::radians?

Modified the test to also test the CL variant.

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.

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

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?

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?

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

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.

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

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

@adam-yang Thank you!

adam-yang · 2024-10-04T17:51:45Z

@farzonl This is ready to check-in if you or @python3kgae don't have any additional comments.

python3kgae · 2024-10-04T18:08:28Z

@farzonl This is ready to check-in if you or @python3kgae don't have any additional comments.

I don't have additional comments.

github-actions · 2024-10-04T18:10:10Z

@adam-yang Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

makes progress on #99151 ### Changes - Added int_dx_radians intrinsic in IntrinsicsDirectX.td - Added expansion for int_dx_radians in DXILIntrinsicExpansion.cpp` - Added DXIL backend test case ### Related PRs * [[clang][HLSL] Add radians intrinsic #110802](#110802) * [[SPIRV] Add radians intrinsic #110800](#110800)

partially fixes #99151 ### Changes * Implemented `radians` clang builtin * Linked `radians` clang builtin with `hlsl_intrinsics.h` * Added sema checks for `radians` to `CheckHLSLBuiltinFunctionCall` in `SemaChecking.cpp` * Add codegen for `radians` to `EmitHLSLBuiltinExpr` in `CGBuiltin.cpp` * Add codegen tests to `clang/test/CodeGenHLSL/builtins/radians.hlsl` * Add sema tests to `clang/test/SemaHLSL/BuiltIns/radians-errors.hlsl` ### Related PRs * [[DXIL] Add radians intrinsic #110616](#110616) * [[SPIRV] Add radians intrinsic #110800](#110800)

This reverts commit c0f8889.

@adam-yang

Reverts #110800 `llvm\test\CodeGen\DirectX\radians.ll` is failing after this change. @adam-yang please send a new PR with the issue resolved once you've had time to investigate.

I had too many tabs open and reverted #110800 by mistake, it was supposed to be #110616. This reverts commit dec6fe3.

bogner · 2024-10-07T16:31:12Z

Note: I reverted this by accident (meant to revert #110616) and then undid the revert in b2c615f

adam-yang changed the title ~~Hlsl radians spirv~~ [SPIRV] Add radians intrinsic Oct 2, 2024

This was referenced Oct 2, 2024

[clang][HLSL] Add radians intrinsic #110802

Merged

[DXIL] Add radians intrinsic #110616

Merged

adam-yang marked this pull request as ready for review October 2, 2024 08:51

llvmbot added backend:SPIR-V llvm:ir labels Oct 2, 2024

farzonl reviewed Oct 2, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

farzonl reviewed Oct 2, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

farzonl reviewed Oct 2, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

farzonl requested changes Oct 2, 2024

View reviewed changes

adam-yang added 3 commits October 2, 2024 15:55

Added radians selection for spirv

010cf37

Added nomem attribute

ea6331e

Fixed bad rebase. Removed selectRadians and using selectExtInst instead

735e8be

adam-yang force-pushed the hlsl-radians-spirv branch from 1c6b14b to 735e8be Compare October 2, 2024 23:41

farzonl approved these changes Oct 2, 2024

View reviewed changes

python3kgae reviewed Oct 3, 2024

View reviewed changes

Added test variant for opencl

0ab8d0f

python3kgae approved these changes Oct 3, 2024

View reviewed changes

Separated the opencl test to its own thing

17175f2

michalpaszkowski approved these changes Oct 4, 2024

View reviewed changes

farzonl merged commit c0f8889 into llvm:main Oct 4, 2024
9 checks passed

bogner added a commit that referenced this pull request Oct 7, 2024

Revert "[SPIRV] Add radians intrinsic (#110800)"

66c986a

This reverts commit c0f8889.

bogner mentioned this pull request Oct 7, 2024

Revert "[SPIRV] Add radians intrinsic" #111398

Merged

bogner added a commit that referenced this pull request Oct 7, 2024

Reapply "[SPIRV] Add radians intrinsic"

b2c615f

I had too many tabs open and reverted #110800 by mistake, it was supposed to be #110616. This reverts commit dec6fe3.

agozillon mentioned this pull request Oct 24, 2024

[DXIL] Add radians intrinsic (#110616) #113546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPIRV] Add radians intrinsic #110800

[SPIRV] Add radians intrinsic #110800

adam-yang commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024

llvmbot commented Oct 2, 2024 •

edited

Loading

Changes

Related PRs

farzonl left a comment

python3kgae Oct 3, 2024

adam-yang Oct 3, 2024

farzonl Oct 3, 2024 •

edited

Loading

python3kgae Oct 3, 2024

python3kgae Oct 3, 2024

farzonl Oct 3, 2024

adam-yang Oct 3, 2024

farzonl Oct 3, 2024

adam-yang Oct 3, 2024

michalpaszkowski Oct 4, 2024

adam-yang commented Oct 4, 2024

python3kgae commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

bogner commented Oct 7, 2024

[SPIRV] Add radians intrinsic #110800

[SPIRV] Add radians intrinsic #110800

Conversation

adam-yang commented Oct 2, 2024 • edited Loading

Changes

Related PRs

github-actions bot commented Oct 2, 2024

llvmbot commented Oct 2, 2024 • edited Loading

Changes

Related PRs

farzonl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farzonl Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-yang commented Oct 4, 2024

python3kgae commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

bogner commented Oct 7, 2024

adam-yang commented Oct 2, 2024 •

edited

Loading

llvmbot commented Oct 2, 2024 •

edited

Loading

farzonl Oct 3, 2024 •

edited

Loading