-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// 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)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// 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( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
NewRegister, PtrType, "placeholder", nullptr, | ||||||
SPIRV::StorageClass::Function, nullptr, true, false, | ||||||
SPIRV::LinkageType::Import, MIRBuilder, false); | ||||||
Comment on lines
+4035
to
+4036
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add comments to the bool params please (https://llvm.org/docs/CodingStandards.html#comment-formatting) ? |
||||||
// 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Not necessary |
||||||
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.... | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
; 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see a test where |
||
%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) |
There was a problem hiding this comment.
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 initOpcode
andSet
? Looks a bit convoluted for no reasonUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
andModfStruct
has a different semantic. I don't have strong opinion whether to generateModfStruct
in this patch right away (up to you to decide), but obviously handling forModfStruct
will be different.BTW, please leave
FIXME
comment in case if you don't want to generateModfStruct
right away.There was a problem hiding this comment.
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 useGL::Modf
at all