Add possibility to lower llvm.fmuladd into mad from OpenCL extinst #824

AlexeySachkov · 2020-11-23T14:23:26Z

This is a PR #793, which I had to re-create in order to enable Travis CI check (I hope it will work)

AlexeySotkin · 2020-11-23T14:34:04Z

lib/SPIRV/SPIRVWriter.cpp

@@ -2231,6 +2231,22 @@ SPIRVValue *LLVMToSPIRV::transIntrinsicInst(IntrinsicInst *II,
    // For llvm.fmuladd.* fusion is not guaranteed. If a fused multiply-add
    // is required the corresponding llvm.fma.* intrinsic function should be
    // used instead.
+    if (!checkTypeForSPIRVExtendedInstLowering(II, BM))


There is no handling for fmuladd in checkTypeForSPIRVExtendedInstLowering, it will always return true. Should we extend checkTypeForSPIRVExtendedInstLowering?

I don't think that this is necessary, because llvm.fmuladd.* is only defined for floating-point scalars, which perfectly matches requirements of fma extinst. We don't even need to check for vector length, because llvm.fmuladd.* is not defined for vectors

checkTypeForSPIRVExtendedInstLowering takes II as a parameter. This function is based on switch on intrinsic id and there is no case for Intrinsic::fmuladd. We either should add case Intrinsic::fmuladd:, or don't call checkTypeForSPIRVExtendedInstLowering at all.

Fair enough, I will remove the call

include/LLVMSPIRVOpts.h

lib/SPIRV/SPIRVWriter.cpp

tools/llvm-spirv/llvm-spirv.cpp

AGindinson · 2020-11-30T13:16:13Z

tools/llvm-spirv/llvm-spirv.cpp

+static cl::opt<bool> SPIRVReplaceLLVMFmulAddWithOpenCLMad(
+    "spirv-replace-fmuladd-with-ocl-mad",
+    cl::desc("Allow replacement of llvm.fmuladd.* intrinsic with OpenCL mad "
+             "instruction from OpenCL extended instruction set"),
+    cl::init(true));


How likely is it that similar user-requested replacements will be added for other intrinsics? If such plans (or such bits of code) exist already, would a single "joint" option like -spirv-replace-intrinsics-with-ocl-ext be preferable?

How likely is it that similar user-requested replacements will be added for other intrinsics?

This patch is more like a workaround for the lack of fine-grained fp-contraction support in SPIR-V spec, comparing it to OpenCL spec and LLVM IR representation. So, in general I wouldn't expect this to happen often, but still there might be a few more similar situations (right now I can't think of any)

If such plans (or such bits of code) exist already, would a single "joint" option like -spirv-replace-intrinsics-with-ocl-ext be preferable?

Since this is more like a workaround and I'm not aware of any similar things I guess it would be better to go with this particular option for fmuladd

AGindinson · 2020-11-30T13:18:27Z

tools/llvm-spirv/llvm-spirv.cpp

@@ -202,6 +202,12 @@ static cl::opt<SPIRV::DebugInfoEIS> DebugEIS(
                   "extended instruction set. This version of SPIR-V debug "
                   "info format is compatible with the SPIRV-Tools")));

+static cl::opt<bool> SPIRVReplaceLLVMFmulAddWithOpenCLMad(


Is there a reason to make it a bool-accepting option instead of a simple flag?
(I personally don't think it matters much, but just for the sake of consistency)

Is there a reason to make it a bool-accepting option instead of a simple flag?

No particular reason from command line interface point of view, but from technical point of view I need to take a more careful look at LLVM's CommandLine library to see if it is possible

Looks like boolOrDefault is what I'm looking for here

Ok, I was too quick with boolOrDefault: I don't need it, because I don't really care about whether the option was passed or not, but I care about its value.

Also, CommandLine library supports flags through boolean options, i.e. -spirv-replace-fmuladd-with-ocl-mad is equivalent to -spirv-replace-fmuladd-with-ocl-mad=true.

From my point of view the current way is good enough: most likely no one will ever use that switch and I don't want to overcomplicate the code just to get a -no-spirv-replace-fmuladd-with-ocl-mad alias (moreover, due to CommandLine library it would be really confusing to be able to use -no-spirv-replace-fmuladd-with-ocl-mad=false)

Thanks for expanding on this, SGTM.

lib/SPIRV/SPIRVWriter.cpp

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

…hronosGroup#824)

)

…hronosGroup#824)

…ad from OpenCL.. 'master' -> 'xmain-web' (KhronosGroup#8) CONFLICT (content): Merge conflict in include/LLVMSPIRVOpts.h commit 1cb18fd Author: Alexey Sachkov <alexey.sachkov@intel.com> Date: Wed Dec 9 17:45:33 2020 +0300 Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (KhronosGroup#824) Change-Id: I725ce530db2dfe1c2f443b70f5d298b9a8238fc0

AlexeySachkov requested a review from AlexeySotkin November 23, 2020 14:23

AlexeySotkin reviewed Nov 23, 2020

View reviewed changes

AlexeySachkov force-pushed the private/asachkov/lower-fmuladd-to-mad branch from e0bba94 to 2b65828 Compare November 26, 2020 13:17

AlexeySachkov mentioned this pull request Nov 26, 2020

Translate llvm.fmuladd to fma SPIRV intrinsic #793

Closed

mlychkov reviewed Nov 30, 2020

View reviewed changes

include/LLVMSPIRVOpts.h Outdated Show resolved Hide resolved

lib/SPIRV/SPIRVWriter.cpp Outdated Show resolved Hide resolved

tools/llvm-spirv/llvm-spirv.cpp Outdated Show resolved Hide resolved

AGindinson reviewed Nov 30, 2020

View reviewed changes

fadeeval and others added 7 commits December 8, 2020 18:57

Translate llvm.fmuladd to fma SPIRV intrinsic

03ab5a2

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

Chage of fma to mad

99edb2d

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

Add formatting

753b8ec

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

Add command line option

e041c5a

Apply clang-format

8f74f6c

Apply comments

f49b8b9

Apply comments

514f5b8

AlexeySachkov force-pushed the private/asachkov/lower-fmuladd-to-mad branch from 05d38e0 to 514f5b8 Compare December 8, 2020 16:00

AlexeySachkov requested a review from AlexeySotkin December 8, 2020 16:00

AlexeySotkin approved these changes Dec 9, 2020

View reviewed changes

AlexeySachkov merged commit 1cb18fd into KhronosGroup:master Dec 9, 2020

MrSidims pushed a commit to MrSidims/SPIRV-LLVM-Translator that referenced this pull request Jan 18, 2021

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (K…

988ccc4

…hronosGroup#824)

MrSidims pushed a commit to MrSidims/SPIRV-LLVM-Translator that referenced this pull request Jan 18, 2021

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (K…

4348904

…hronosGroup#824)

MrSidims pushed a commit to MrSidims/SPIRV-LLVM-Translator that referenced this pull request Jan 19, 2021

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (K…

b197de7

…hronosGroup#824)

AlexeySachkov added a commit that referenced this pull request Jan 20, 2021

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (#824

24a26ee

)

AlexeySachkov added a commit that referenced this pull request Feb 4, 2021

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (#824

c05e1d6

)

AlexeySachkov mentioned this pull request Aug 3, 2021

LLVM 12's SLP Vectorizer introduces unhandled llvm.masked.gather calls #1139

Open

DmitryBushev pushed a commit to DmitryBushev/SPIRV-LLVM-Translator that referenced this pull request Sep 1, 2021

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst (K…

c822428

…hronosGroup#824)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst #824

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst #824

AlexeySachkov commented Nov 23, 2020

AlexeySotkin Nov 23, 2020

AlexeySachkov Nov 30, 2020

AlexeySotkin Dec 3, 2020

AlexeySachkov Dec 8, 2020

AGindinson Nov 30, 2020

AlexeySachkov Nov 30, 2020 •

edited

Loading

AGindinson Nov 30, 2020

AlexeySachkov Nov 30, 2020

AlexeySachkov Nov 30, 2020

AlexeySachkov Nov 30, 2020

AGindinson Nov 30, 2020

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst #824

Add possibility to lower llvm.fmuladd into mad from OpenCL extinst #824

Conversation

AlexeySachkov commented Nov 23, 2020

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

AlexeySachkov Nov 30, 2020 • 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

AlexeySachkov Nov 30, 2020 •

edited

Loading