Skip to content

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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

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
5bfcf33
Implement translation for llvm.modf.* intrinsics.
maarquitos14 Jul 8, 2025
7d87ed3
Add testing for llvm.modf.* intrinsics.
maarquitos14 Jul 8, 2025
e07f915
Address code review feedback.
maarquitos14 Jul 15, 2025
cdc2571
Fix clang-format issue.
maarquitos14 Jul 15, 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
76 changes: 76 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 @@ -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>
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@MrSidims MrSidims Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (STI.canUseExtInstSet(Set)) {
MachineIRBuilder MIRBuilder(I);
// Get pointer type for alloca variable.
const SPIRVType *PtrType = GR.getOrCreateSPIRVPointerType(
ResType, MIRBuilder, SPIRV::StorageClass::Input);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this!

// 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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in last commit.

// 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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 changed the approach to use variables local to function instead of global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

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....
Expand Down
22 changes: 22 additions & 0 deletions llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a spirv-val check for the test as well, the pointer was ill-formed and nothing caught it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I assumed the test had a spirv-val check already, but it didn't.

; 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a test where llvm.modf is called outside the entry block (SPIR-V mandates the OpVariable to be in the entry block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in last commit.

%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)
Loading