Add emulation of (s/u)_(add/sub)_sat intrinsics if needed #2138

MrSidims · 2023-08-25T13:43:13Z

While the translator was built around OpenCL - other targets that support SPIR-V and don't support OpenCL might use it.

This patch adds --spirv-use-ocl-math-for-llvm-intrinsic option to control whether we should translate them as math intrinsics to OpenCL ext math instructions or emulate. Default is true aka translate as math instructions.

I don't really want to end up implementing Quake's sqrt algorithm, but it's a possible scenario as well.

Plans are:

merge existing --spirv-replace-fmuladd-with-ocl-mad with the new option;
introduce InstCombine pass in reverse translation under an option.

While the translator was built around OpenCL - other targets that support SPIR-V and don't support OpenCL might use it. This patch adds --spirv-use-ocl-math-for-llvm-intrinsic option to control whether we should translate them as math intrinsics to OpenCL ext math instructions or emulate. Default is true aka translate as math instructions. I don't really want to end up implementing Quake's sqrt algorithm, but it's a possible scenario as well. Plans are: 1. merge existing --spirv-replace-fmuladd-with-ocl-mad with the new option; 2. optionally introduce InstCombine pass in reverse translation. Signed-off-by: Sidorov, Dmitry <[email protected]>

MrSidims · 2023-08-25T13:46:56Z

@LU-JOHN @asudarsa @maksimsab please take a look

lib/SPIRV/SPIRVWriter.cpp

Signed-off-by: Sidorov, Dmitry <[email protected]>

test/llvm-intrinsics/add_sub.sat.ll

lib/SPIRV/SPIRVWriter.cpp

asudarsa · 2023-08-26T03:22:07Z

lib/SPIRV/SPIRVWriter.cpp

+      SPIRVValue *Add =
+          BM->addBinaryInst(OpIAdd, Ty, FirstArgVal, SecondArgVal, BB);
+      SPIRVValue *Cmp =
+          BM->addCmpInst(OpUGreaterThan, SPVBoolTy, Add, FirstArgVal, BB);


I think OpUGreaterThan is the right op. But, I am not able to reason out with an example. Can you please provide one if possible? Not a PR blocker.

Thanks

Should be OpUGreaterThanEqual according to your comment from above
I've also checked generated IR after the translation by running inst combine - it's able to restore uadd_sat intrinsic

lib/SPIRV/SPIRVWriter.cpp

asudarsa · 2023-08-26T03:49:48Z

lib/SPIRV/SPIRVWriter.cpp

+          BM->addBinaryInst(OpISub, Ty, Max, SecondArgVal, BB);
+      SPIRVValue *CanPosOverflow =
+          BM->addCmpInst(OpSGreaterThan, SPVBoolTy, FirstArgVal, MaxSubB, BB);
+      SPIRVValue *PosOverflow = BM->addInstTemplate(


Not sure if the order of the operand could impact performance here. Again just a request for quick check. Not a PR blocker.

Thanks

TBH I'm not expecting this implementation be performant at all. Ideally we should generate such code that is collapsing to sadd/sub_sat intrinsic after we run InstCombine , but it's not happening. That is because InstCombine checks overflow by extending integer to a larger one and comparing the result with MAX/MIN of the previous type. We can't do this in general case, because we can't cast integer to a wider one in SPIR-V without extensions.

test/llvm-intrinsics/add_sub.sat.ll

asudarsa

One possible correction required. Please address. Thanks for this change.

asudarsa · 2023-08-26T03:59:55Z

Title can be: Improve translation support for (s/u)_(add/sub)_sat intrinsics

asudarsa · 2023-08-26T04:02:17Z

tools/llvm-spirv/llvm-spirv.cpp

@@ -258,6 +258,12 @@ static cl::opt<SPIRV::BuiltinFormat> SPIRVBuiltinFormat(
        clEnumValN(SPIRV::BuiltinFormat::Global, "global",
                   "Use globals to represent SPIR-V builtin variables")));

+static cl::opt<bool> SPIRVUseOpenCLExtInstructionsForLLVMIntrinsic(


I feel that this option sounds very generic. It sounds as if every llvm intrinsic is covered under this option. Can you please elaborate?

Thanks

Renamed to SPIRVUseOpenCLExtInstructionsForLLVMMathIntrinsic

Signed-off-by: Sidorov, Dmitry <[email protected]>

asudarsa

LGTM.Thanks

MrSidims · 2023-08-28T16:59:17Z

tools/llvm-spirv/llvm-spirv.cpp

@@ -258,6 +258,12 @@ static cl::opt<SPIRV::BuiltinFormat> SPIRVBuiltinFormat(
        clEnumValN(SPIRV::BuiltinFormat::Global, "global",
                   "Use globals to represent SPIR-V builtin variables")));

+static cl::opt<bool> SPIRVUseOpenCLExtInstructionsForLLVMMathIntrinsic(
+    "spirv-use-ocl-for-llvm-math-intrinsic", cl::init(true),


@svenvh may I ask you to take a look if this option is OK?

I wonder if we can generalize the option? If I understand correctly, you essentially want to control whether the translator is allowed to emit OpenCL.std OpExtInst instructions?

Yeah, exactly. So is your suggesting to rename it or something different? We kinda can find counterparts of https://registry.khronos.org/SPIR-V/specs/unified1/OpenCL.ExtendedInstructionSet.100.html in https://llvm.org/docs/LangRef.html#standard-c-c-library-intrinsics , so would it be spirv-use-std-ocl-for-llvm-std-intrinsics ?

The interface would be similar to --spirv-ext. So for your use case it could look like --spirv-ext-inst=-OpenCL.std for example.

It may require a bit of thinking/refactoring first though, so I don't want to hold up this patch.

Mmm, I see. I will spend some time thinking about it, I'd prefer not to add an option (and enable it in some compiler's frontend driver), only to remove it in a couple of weeks, replacing with a better solution :) So we can keep PR open for a while

svenvh · 2023-08-30T14:19:31Z

I don't really want to end up implementing Quake's sqrt algorithm, but it's a possible scenario as well.

The prospect of growing a math library inside llvm-spirv doesn't sound exciting indeed... ;-)

LU-JOHN · 2023-09-07T20:35:06Z

lib/SPIRV/SPIRVWriter.cpp

+        BM->addCmpInst(OpSLessThan, SPVBoolTy, SecondArgVal, Zero, BB);
+
+    if (IID == Intrinsic::sadd_sat) {
+      // sadd.sat(a, b) -> if (b > 0) && a > MAX - b => overflow -> MAX


Can we use this sequence:

res = a+b
if (b>0 && a>res) res = MAX
if (b<0 && a<res) res = MIN

This makes it unnecessary to calculate "MAX - b" and "MIN - b"

Another implementation with a single comparison:

sum = a+b; if ( ( ~(a^b) & // addends have same sign (sum^a) // result has different sign ) // test MSB < 0) sum = (0x7fffffff^(a>>31)); // saturated result

LU-JOHN · 2023-09-07T20:40:05Z

2. introduce InstCombine pass in reverse translation under an option.

Some of the emulation sequences are long. Could they be modified (e.g. if we know an addend is always positive) or re-ordered so that InstCombine cannot recognize the sequence anymore?

LU-JOHN · 2023-09-07T21:33:30Z

lib/SPIRV/SPIRVWriter.cpp

+      SPIRVValue *FirstSelect = BM->addSelectInst(PosOverflow, Max, Add, BB);
+      return BM->addSelectInst(NegOverflow, Min, FirstSelect, BB);
+    }
+    // ssub.sat(a, b) -> if (b > 0) && a < MIN + b => overflow -> MIN


This can be:

res = a-b
if (b<0 && a>res) res = MAX
if (b>0 && a<res) res = MIN

This makes it unnecessary to calculate "MAX + b" and "MIN + b"

maksimsab · 2023-10-25T12:42:05Z

include/LLVMSPIRVOpts.h

@@ -193,6 +193,14 @@ class TranslatorOpts {
    ReplaceLLVMFmulAddWithOpenCLMad = Value;
  }

+  void setUseOpenCLExtInstructionsForLLVMMathIntrinsic(bool Value) noexcept {


nit regarding noexcept:
LLVM itself prohibits C++ exceptions and they are turned off in CMakeLists by compiler flag -fno-exceptions.
If we use exceptions in translator, then it would be better to stop doing it and start using the compiler flag -fno-exceptions as well.

MrSidims requested review from svenvh and vmaksimo August 25, 2023 13:43

MrSidims changed the title ~~Add emulation of {s/u)_(add/sub)_sat intrinsics if needed~~ Add emulation of (s/u)_(add/sub)_sat intrinsics if needed Aug 25, 2023

MrSidims commented Aug 25, 2023

View reviewed changes

lib/SPIRV/SPIRVWriter.cpp Show resolved Hide resolved

format

56bfd0b

Signed-off-by: Sidorov, Dmitry <[email protected]>

vmaksimo reviewed Aug 25, 2023

View reviewed changes

test/llvm-intrinsics/add_sub.sat.ll Outdated Show resolved Hide resolved

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

asudarsa reviewed Aug 26, 2023

View reviewed changes

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

asudarsa reviewed Aug 26, 2023

View reviewed changes

test/llvm-intrinsics/add_sub.sat.ll Outdated Show resolved Hide resolved

asudarsa requested changes Aug 26, 2023

View reviewed changes

asudarsa reviewed Aug 26, 2023

View reviewed changes

MrSidims added 2 commits August 28, 2023 04:44

Address comments and fix uadd

196464d

Signed-off-by: Sidorov, Dmitry <[email protected]>

Fix test

15705fa

Signed-off-by: Sidorov, Dmitry <[email protected]>

asudarsa approved these changes Aug 28, 2023

View reviewed changes

MrSidims commented Aug 28, 2023

View reviewed changes

LU-JOHN reviewed Sep 7, 2023

View reviewed changes

maksimsab reviewed Oct 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add emulation of (s/u)_(add/sub)_sat intrinsics if needed #2138

Add emulation of (s/u)_(add/sub)_sat intrinsics if needed #2138

MrSidims commented Aug 25, 2023 •

edited

Loading

MrSidims commented Aug 25, 2023

asudarsa Aug 26, 2023

MrSidims Aug 28, 2023

asudarsa Aug 26, 2023

MrSidims Aug 28, 2023

asudarsa left a comment

asudarsa commented Aug 26, 2023

asudarsa Aug 26, 2023

MrSidims Aug 28, 2023 •

edited

Loading

asudarsa left a comment

MrSidims Aug 28, 2023

svenvh Aug 30, 2023

MrSidims Aug 30, 2023

svenvh Aug 30, 2023

MrSidims Aug 30, 2023

svenvh commented Aug 30, 2023

LU-JOHN Sep 7, 2023 •

edited

Loading

LU-JOHN Sep 12, 2023 •

edited

Loading

LU-JOHN commented Sep 7, 2023

LU-JOHN Sep 7, 2023

maksimsab Oct 25, 2023

Add emulation of (s/u)_(add/sub)_sat intrinsics if needed #2138

Are you sure you want to change the base?

Add emulation of (s/u)_(add/sub)_sat intrinsics if needed #2138

Conversation

MrSidims commented Aug 25, 2023 • edited Loading

MrSidims commented Aug 25, 2023

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

asudarsa left a comment

Choose a reason for hiding this comment

asudarsa commented Aug 26, 2023

Choose a reason for hiding this comment

MrSidims Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

asudarsa left a 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

Choose a reason for hiding this comment

svenvh commented Aug 30, 2023

LU-JOHN Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

LU-JOHN Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

LU-JOHN commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims commented Aug 25, 2023 •

edited

Loading

MrSidims Aug 28, 2023 •

edited

Loading

LU-JOHN Sep 7, 2023 •

edited

Loading

LU-JOHN Sep 12, 2023 •

edited

Loading