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.

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

Add fast math flag translation for OpenCL std lib #2762

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

Conversation

MrSidims
Copy link
Contributor

Such possibility was added in SPIR-V 1.6.
This patch also introduces limited translation of nofpclass LLVM parameter attribute.

Such possibility was added in SPIR-V 1.6.
This patch also introduces limited translation of nofpclass LLVM parameter
attribute.

Signed-off-by: Sidorov, Dmitry <[email protected]>
// Handle nofpclass attribute. Nothing to do if fast math flag is already
// set.
if ((BV->isExtInst() && static_cast<SPIRVExtInst *>(
BV)->getExtSetKind() == SPIRVEIS_OpenCL) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we should also check, if fast math flags are applicable to a particular function from OpenCL std lib according to environmental spec, but I beg your pardon for not doing that.

Signed-off-by: Sidorov, Dmitry <[email protected]>
@@ -3038,7 +3038,8 @@ bool LLVMToSPIRVBase::transDecoration(Value *V, SPIRVValue *BV) {
if (Opcode == Instruction::FAdd || Opcode == Instruction::FSub ||
Opcode == Instruction::FMul || Opcode == Instruction::FDiv ||
Opcode == Instruction::FRem ||
((Opcode == Instruction::FNeg || Opcode == Instruction::FCmp) &&
((Opcode == Instruction::FNeg || Opcode == Instruction::FCmp ||
Copy link
Contributor

Choose a reason for hiding this comment

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

@MrSidims, could you check if SPIRVReader handles fast-math flags for FNeg and FCmp correctly, please?
If not, please, file a tracker to follow-up.

Unfortunately, SPIR-V format is not able to translate LLVM semantics correctly. @bashbaug, SPIR-V doesn't allow applying fast-math flags to OpSelect and OpCall instructions, but LLVM IR can have fast math flags attached to select and call instructions. Do you know if this is going to be addressed by future SPIR-V versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SPIR-V doesn't allow applying fast-math flags to OpSelect and OpCall instructions, but LLVM IR can have fast math flags attached to select and call instructions

This is a part of SPV_KHR_float_controls2:

Replace the text following "Only valid on …​" with:
All core instructions which use any floating-point type for either operands or result.
OpExInst extended instructions, where expressly permitted by the extended instruction set in use.

It may be a bit too much and some instructions can or should be excluded from the list, but it is what it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given:

OpExInst extended instructions, where expressly permitted by the extended instruction set in use.

Do we need some updates to the OpenCL Extended Instruction set, or perhaps the OpenCL SPIR-V environment spec? Feel free to file a SPIR-V or OpenCL issue for tracking, if so, so this doesn't slip through the cracks.

We should also add some targeted CTS tests for this functionality, even if they aren't exhaustive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you check if SPIRVReader handles fast-math flags for FNeg and FCmp correctly, please

@bader according to passing tests https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/transcoding/fcmp.ll and https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/transcoding/fneg.ll it handles correctly. Manual run also doesn't show anything fishy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need some updates to the OpenCL Extended Instruction set, or perhaps the OpenCL SPIR-V environment spec?

@bashbaug no, https://registry.khronos.org/SPIR-V/specs/unified1/OpenCL.ExtendedInstructionSet.100.pdf already contains the required lines like:

For environments that allow use of FPFastMathMode decorations on OpExtInst instructions,
FPFastMathMode decorations may be applied to the math instructions.

and there are other several references about it.
But I believe Alexey had another question related to select and call instructions.

We should also add some targeted CTS tests for this functionality, even if they aren't exhaustive.

We should, https://github.com/KhronosGroup/OpenCL-CTS/pulls?q=is%3Apr+is%3Aopen+FPFastMathMode search shows nothing

Signed-off-by: Sidorov, Dmitry <[email protected]>
@@ -3038,7 +3038,8 @@ bool LLVMToSPIRVBase::transDecoration(Value *V, SPIRVValue *BV) {
if (Opcode == Instruction::FAdd || Opcode == Instruction::FSub ||
Opcode == Instruction::FMul || Opcode == Instruction::FDiv ||
Opcode == Instruction::FRem ||
((Opcode == Instruction::FNeg || Opcode == Instruction::FCmp) &&
((Opcode == Instruction::FNeg || Opcode == Instruction::FCmp ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants