[SPIRV] Implement translation for llvm.modf.* intrinsics #147556

maarquitos14 · 2025-07-08T15:45:14Z

Based on KhronosGroup/SPIRV-LLVM-Translator#3100, I'm adding translation for llvm.modf.* intrinsics.

maarquitos14 · 2025-07-08T15:45:28Z

@MrSidims please take a look :)

llvmbot · 2025-07-08T15:46:00Z

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

Author: Marcos Maronas (maarquitos14)

Changes

Based on KhronosGroup/SPIRV-LLVM-Translator#3100, I'm adding translation for llvm.modf.* intrinsics.

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

2 Files Affected:

(modified) llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp (+76)
(modified) llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll (+22)

diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 40a0bd97adaf9..ad6025804608d 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -296,6 +296,8 @@ class SPIRVInstructionSelector : public InstructionSelector {
   bool selectImageWriteIntrinsic(MachineInstr &I) const;
   bool selectResourceGetPointer(Register &ResVReg, const SPIRVType *ResType,
                                 MachineInstr &I) const;
+  bool selectModf(Register ResVReg, const SPIRVType *ResType,
+                  MachineInstr &I) const;
 
   // Utilities
   std::pair<Register, bool>
@@ -3207,6 +3209,9 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
   case Intrinsic::spv_discard: {
     return selectDiscard(ResVReg, ResType, I);
   }
+  case Intrinsic::modf: {
+    return selectModf(ResVReg, ResType, I);
+  }
   default: {
     std::string DiagMsg;
     raw_string_ostream OS(DiagMsg);
@@ -3990,6 +3995,77 @@ bool SPIRVInstructionSelector::selectLog10(Register ResVReg,
                        .constrainAllUses(TII, TRI, RBI);
 }
 
+bool SPIRVInstructionSelector::selectModf(Register ResVReg,
+                                          const SPIRVType *ResType,
+                                          MachineInstr &I) const {
+  // llvm.modf has a single arg --the number to be decomposed-- and returns a
+  // struct { restype, restype }, while OpenCLLIB::modf has two args --the
+  // number to be decomposed and a pointer--, returns the fractional part and
+  // the integral part is stored in the pointer argument. Therefore, we can't
+  // use directly the OpenCLLIB::modf intrinsic. However, we can do some
+  // scaffolding to make it work. The idea is to create an alloca instruction
+  // to get a ptr, pass this ptr to OpenCL::modf, and then load the value
+  // from this ptr to place it in the struct. llvm.modf returns the fractional
+  // part as the first element of the result, and the integral part as the
+  // second element of the result.
+
+  // At this point, the return type is not a struct anymore, but rather two
+  // independent elements of SPIRVResType. We can get each independent element
+  // from I.getDefs() or I.getOperands().
+  ExtInstList ExtInsts = {{SPIRV::InstructionSet::OpenCL_std, CL::modf},
+                          {SPIRV::InstructionSet::GLSL_std_450, GL::Modf}};
+  for (const auto &Ex : ExtInsts) {
+    SPIRV::InstructionSet::InstructionSet Set = Ex.first;
+    uint32_t Opcode = Ex.second;
+    if (STI.canUseExtInstSet(Set)) {
+      MachineIRBuilder MIRBuilder(I);
+      // Get pointer type for alloca variable.
+      const SPIRVType *PtrType = GR.getOrCreateSPIRVPointerType(
+          ResType, MIRBuilder, SPIRV::StorageClass::Input);
+      // Create new register for the pointer type of alloca variable.
+      Register NewRegister =
+          MIRBuilder.getMRI()->createVirtualRegister(&SPIRV::iIDRegClass);
+      MIRBuilder.getMRI()->setType(NewRegister, LLT::pointer(0, 64));
+      // Assign SPIR-V type of the pointer type of the alloca variable to the
+      // new register.
+      GR.assignSPIRVTypeToVReg(PtrType, NewRegister, MIRBuilder.getMF());
+      // Build the alloca variable.
+      Register Variable = GR.buildGlobalVariable(
+          NewRegister, PtrType, "placeholder", nullptr,
+          SPIRV::StorageClass::Function, nullptr, true, false,
+          SPIRV::LinkageType::Import, MIRBuilder, false);
+      // Modf must have 4 operands, the first two are the 2 parts of the result,
+      // the third is the operand, and the last one is the floating point value.
+      assert(I.getNumOperands() == 4 &&
+             "Expected 4 operands for modf instruction");
+      MachineBasicBlock &BB = *I.getParent();
+      // Create the OpenCLLIB::modf instruction.
+      auto MIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpExtInst))
+                     .addDef(ResVReg)
+                     .addUse(GR.getSPIRVTypeID(ResType))
+                     .addImm(static_cast<uint32_t>(Set))
+                     .addImm(Opcode)
+                     .setMIFlags(I.getFlags())
+                     .add(I.getOperand(3)) // Floating point value.
+                     .addUse(Variable);    // Pointer to integral part.
+      // Assign the integral part stored in the ptr to the second element of the
+      // result.
+      Register IntegralPartReg = I.getOperand(1).getReg();
+      if (IntegralPartReg.isValid()) {
+        // Load the value from the pointer to integral part.
+        auto LoadMIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpLoad))
+                           .addDef(IntegralPartReg)
+                           .addUse(GR.getSPIRVTypeID(ResType))
+                           .addUse(Variable);
+        return LoadMIB.constrainAllUses(TII, TRI, RBI);
+      }
+
+      return MIB.constrainAllUses(TII, TRI, RBI);
+    }
+  }
+  return false;
+}
+
 // Generate the instructions to load 3-element vector builtin input
 // IDs/Indices.
 // Like: GlobalInvocationId, LocalInvocationId, etc....
diff --git a/llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll b/llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll
index 3d46b527bf14f..91c6174e63d77 100644
--- a/llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll
+++ b/llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll
@@ -337,3 +337,25 @@ entry:
 }
 
 declare float @llvm.fma.f32(float, float, float)
+
+; CHECK: OpFunction
+; CHECK: %[[#d:]] = OpFunctionParameter %[[#]]
+; CHECK: %[[#fracPtr:]] = OpFunctionParameter %[[#]]
+; CHECK: %[[#integralPtr:]] = OpFunctionParameter %[[#]]
+; CHECK: %[[#varPtr:]] = OpVariable %[[#]] Function
+; CHECK: %[[#frac:]] = OpExtInst %[[#var2]] %[[#extinst_id]] modf %[[#d]] %[[#varPtr]]
+; CHECK: %[[#integral:]] = OpLoad %[[#var2]] %[[#varPtr]]
+; CHECK: OpStore %[[#fracPtr]] %[[#frac]]
+; CHECK: OpStore %[[#integralPtr]] %[[#integral]]
+; CHECK: OpFunctionEnd
+define void @foo(double %d, ptr addrspace(1) %frac, ptr addrspace(1) %integral) {
+entry:
+  %4 = tail call { double, double } @llvm.modf.f64(double %d)
+  %5 = extractvalue { double, double } %4, 0
+  %6 = extractvalue { double, double } %4, 1
+  store double %5, ptr addrspace(1) %frac, align 8
+  store double %6, ptr addrspace(1) %integral, align 8
+  ret void
+}
+
+declare { double, double } @llvm.modf.f64(double)

Naghasan · 2025-07-08T16:02:34Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+  for (const auto &Ex : ExtInsts) {
+    SPIRV::InstructionSet::InstructionSet Set = Ex.first;
+    uint32_t Opcode = Ex.second;


why a loop and no a if / else to init Opcode and Set ? Looks a bit convoluted for no reason

Agree with Victor, that it's probably better to use if/else here, but for a different reason. From https://registry.khronos.org/SPIR-V/specs/unified1/GLSL.std.450.html : Modf is deprecated, use ModfStruct instead. and ModfStruct has a different semantic. I don't have strong opinion whether to generate ModfStruct in this patch right away (up to you to decide), but obviously handling for ModfStruct will be different.

BTW, please leave FIXME comment in case if you don't want to generate ModfStruct right away.

well spotted, ModfStruct was added in version 0.99, so IMO there is no reason to use GL::Modf at all

Honestly, I just copied the structure from SPIRVInstructionSelector::selectExtInst and adapted it for this particular case, but I do agree it seems convoluted. Also, if GL::Modf is deprecated there's no reason to keep this. I'll simplify it, thanks for catching this.

Naghasan · 2025-07-09T06:53:46Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+      MachineIRBuilder MIRBuilder(I);
+      // Get pointer type for alloca variable.
+      const SPIRVType *PtrType = GR.getOrCreateSPIRVPointerType(
+          ResType, MIRBuilder, SPIRV::StorageClass::Input);


Suggested change

ResType, MIRBuilder, SPIRV::StorageClass::Input);

ResType, MIRBuilder, SPIRV::StorageClass::Function);

Thanks for catching this!

llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

Naghasan · 2025-07-09T07:07:14Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+      // new register.
+      GR.assignSPIRVTypeToVReg(PtrType, NewRegister, MIRBuilder.getMF());
+      // Build the alloca variable.
+      Register Variable = GR.buildGlobalVariable(


I think this works because you have 1 call to the intrinsics in your test, this is the first thing the function does

Module *M = MIRBuilder.getMF().getFunction().getParent(); GVar = M->getGlobalVariable(Name); if (GVar == nullptr) { const Type *Ty = getTypeForSPIRVType(BaseType); // TODO: check type. // Module takes ownership of the global var. GVar = new GlobalVariable(*M, const_cast<Type *>(Ty), false, GlobalValue::ExternalLinkage, nullptr, Twine(Name)); }

You probably going to end up with strange things if you have 2 calls in 2 distinct functions.

I changed the approach to use variables local to function instead of global.

Also, worth to note I extended the test to have a second function calling llvm.modf and it works well.

llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll

Naghasan · 2025-07-09T07:10:28Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+      // Create new register for the pointer type of alloca variable.
+      Register NewRegister =
+          MIRBuilder.getMRI()->createVirtualRegister(&SPIRV::iIDRegClass);
+      MIRBuilder.getMRI()->setType(NewRegister, LLT::pointer(0, 64));


Suggested change

MIRBuilder.getMRI()->setType(NewRegister, LLT::pointer(0, 64));

MIRBuilder.getMRI()->setType(NewRegister, LLT::pointer(storageClassToAddressSpace(SPIRV::StorageClass::Function), GR->getPointerSize()));

Done in last commit.

MrSidims · 2025-07-09T12:57:47Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+      assert(I.getNumOperands() == 4 &&
+             "Expected 4 operands for modf instruction");
+      MachineBasicBlock &BB = *I.getParent();
+      // Create the OpenCLLIB::modf instruction.


nit: Not necessary OpenCLLIB:: as the code also handles GLSL_std_450

I have now moved to if/else approach, so I kept this as is for the OpenCL branch. Let me know if that's okay with you.

MrSidims · 2025-07-09T13:03:25Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+  for (const auto &Ex : ExtInsts) {
+    SPIRV::InstructionSet::InstructionSet Set = Ex.first;
+    uint32_t Opcode = Ex.second;


Agree with Victor, that it's probably better to use if/else here, but for a different reason. From https://registry.khronos.org/SPIR-V/specs/unified1/GLSL.std.450.html : Modf is deprecated, use ModfStruct instead. and ModfStruct has a different semantic. I don't have strong opinion whether to generate ModfStruct in this patch right away (up to you to decide), but obviously handling for ModfStruct will be different.

BTW, please leave FIXME comment in case if you don't want to generate ModfStruct right away.

github-actions · 2025-07-15T20:39:45Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

Naghasan

Nit pick the name and add the ability to skip the OpLabel if it is there

Naghasan · 2025-07-17T13:47:38Z

llvm/lib/Target/SPIRV/SPIRVUtils.cpp

@@ -995,4 +995,26 @@ unsigned getArrayComponentCount(const MachineRegisterInfo *MRI,
  return foldImm(ResType->getOperand(2), MRI);
 }

+MachineBasicBlock::iterator
+getPosForOpVariableWithinBlock(MachineBasicBlock &BB) {


Suggested change

getPosForOpVariableWithinBlock(MachineBasicBlock &BB) {

getFirstValidInstructionInsertPoint(MachineBasicBlock &BB) {

Done in 435d636

Naghasan · 2025-07-17T13:48:23Z

llvm/lib/Target/SPIRV/SPIRVUtils.cpp

+  }
+  // VarPos is now pointing at after the last OpFunctionParameter, if any,
+  // or after OpFunction, if no parameters.
+  return VarPos;


Suggested change

return VarPos;

return VarPos != BB.end() && VarPos->getOpcode() == SPIRV::OpLabel ? ++VarPos : VarPos;

Done in 435d636, but it should never happen, OpLabel is generated later.

Naghasan · 2025-07-17T13:48:37Z

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

+    GR.assignSPIRVTypeToVReg(PtrType, PtrTyReg, MIRBuilder.getMF());
+    MachineBasicBlock &EntryBB = I.getMF()->front();
+    MachineBasicBlock::iterator VarPos =
+        getPosForOpVariableWithinBlock(EntryBB);


Suggested change

getPosForOpVariableWithinBlock(EntryBB);

getFirstValidInstructionInsertPoint(EntryBB);

Done in 435d636

Based on KhronosGroup/SPIRV-LLVM-Translator#3100, I'm adding translation for `llvm.modf.*` intrinsics.

maarquitos14 added 2 commits July 8, 2025 17:08

Implement translation for llvm.modf.* intrinsics.

5bfcf33

Add testing for llvm.modf.* intrinsics.

7d87ed3

llvmbot added the backend:SPIR-V label Jul 8, 2025

Naghasan requested changes Jul 9, 2025

View reviewed changes

MrSidims reviewed Jul 9, 2025

View reviewed changes

Address code review feedback.

e07f915

Fix clang-format issue.

cdc2571

maarquitos14 requested review from Naghasan and MrSidims July 16, 2025 11:18

Naghasan reviewed Jul 16, 2025

View reviewed changes

maarquitos14 added 3 commits July 17, 2025 11:17

Address code review feedback.

4a17c21

Add checks for generated SPIR-V.

e803ef6

Fix clang-format issue.

4f91ce4

maarquitos14 requested a review from Naghasan July 17, 2025 09:40

Naghasan approved these changes Jul 17, 2025

View reviewed changes

Address code review feedback.

435d636

Naghasan approved these changes Jul 17, 2025

View reviewed changes

MrSidims approved these changes Jul 21, 2025

View reviewed changes

MrSidims merged commit 1d02de2 into llvm:main Jul 21, 2025
10 checks passed

This was referenced Jul 23, 2025

test abhinavgaba/llvm-project#2

Closed

Add dataFence plugin interface abhinavgaba/llvm-project#3

Closed

mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025

[SPIRV] Implement translation for llvm.modf.* intrinsics (llvm#147556)

9e3ed2d

Based on KhronosGroup/SPIRV-LLVM-Translator#3100, I'm adding translation for `llvm.modf.*` intrinsics.

maarquitos14 added a commit to maarquitos14/llvm that referenced this pull request Aug 6, 2025

Cherry-pick llvm/llvm-project#147556

c1b3e37

	ResType, MIRBuilder, SPIRV::StorageClass::Input);
	ResType, MIRBuilder, SPIRV::StorageClass::Function);

	MIRBuilder.getMRI()->setType(NewRegister, LLT::pointer(0, 64));
	MIRBuilder.getMRI()->setType(NewRegister, LLT::pointer(storageClassToAddressSpace(SPIRV::StorageClass::Function), GR->getPointerSize()));

	getPosForOpVariableWithinBlock(MachineBasicBlock &BB) {
	getFirstValidInstructionInsertPoint(MachineBasicBlock &BB) {

	return VarPos;
	return VarPos != BB.end() && VarPos->getOpcode() == SPIRV::OpLabel ? ++VarPos : VarPos;

	getPosForOpVariableWithinBlock(EntryBB);
	getFirstValidInstructionInsertPoint(EntryBB);

[SPIRV] Implement translation for llvm.modf.* intrinsics #147556

[SPIRV] Implement translation for llvm.modf.* intrinsics #147556

Conversation

maarquitos14 commented Jul 8, 2025

Uh oh!

maarquitos14 commented Jul 8, 2025

Uh oh!

llvmbot commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrSidims Jul 9, 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!

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!

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!

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!

MrSidims Jul 9, 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!

github-actions bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Naghasan left a comment

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!

Uh oh!

Uh oh!

MrSidims Jul 9, 2025 •

edited

Loading

MrSidims Jul 9, 2025 •

edited

Loading

github-actions bot commented Jul 15, 2025 •

edited

Loading