[SPIR-V Extension] fpbuiltin-max-error support #2056

asudarsa · 2023-06-19T01:17:53Z

This changes add SPIR-V translator support for the SPIR-V extension documented here: KhronosGroup/SPIRV-Registry#193.
This extension adds one decoration to represent maximum error for FP operations and adds the related Capability.
SPIRV Headers support for representing this in SPIR-V: KhronosGroup/SPIRV-Headers#363

intel/llvm#8134 added a new call-site attribute associated with FP builtin intrinsics. This attribute is named 'fpbuiltin-max-error'.
Following example shows how this extension is supported in the translator. The input LLVM IR uses new LLVM builtin calls to represent FP operations. An attribute named 'fpbuiltin-max-error' is used to represent the max-error allowed in the FP operation.
Example
Input LLVM:
%t6 = call float @llvm.fpbuiltin.sin.f32(float %f1) #2
attributes #2 = { "fpbuiltin-max-error"="2.5" }

This is translated into a SPIR-V instruction (for add/sub/mul/div/rem) and OpenCl extended instruction for other instructions. A decoration to represent the max-error is attached to the SPIR-V instruction.

SPIR-V code:
4 Decorate 97 FPMaxErrorDecorationINTEL 1075838976
6 ExtInst 2 97 1 sin 88

No new support is added to support translating this SPIR_V back to LLVM. Existing support is used. The decoration is translated back into named metadata associated with the LLVM instruction. This can be readily consumed by backends.

Based on input from @andykaylor, we emit attributes when the FP operation is translated back to a call to a builtin function and emit metadata otherwise.

Translated LLVM code for basic math functions (add/sub/mul/div/rem):
%t6 = fmul float %f1, %f2, !fpbuiltin-max-error !7
!7 = !{!"2.500000"}

Translated LLVM code for other math functions:
%t6 = call spir_func float @_Z3sinf(float %f1) #3
attributes #3 = { "fpbuiltin-max-error"="4.000000" }

Thanks

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

…port_for_SPV_INTEL_fp_max_error_spec_extension

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa · 2023-06-19T01:45:27Z

Draft mode till SPIR-V Header changes get checked in.

Thanks

asudarsa · 2023-06-19T01:54:02Z

@MrSidims, @vmaksimo, @maksimsab. @LU-JOHN, @jgstarIntel
Please take a look when convenient.

Thanks

andykaylor · 2023-06-20T18:03:00Z

I think we've talked about this, but I can't remember the reasoning. Can you tell me why we need to translate this to metadata and not an attribute at the call site when translating from SPIR-V back to LLVM IR? The metadata is subject to being dropped, which would change the semantics of the call.

If we must use metadata, the standard "fpmath" metadata has the same meaning. We decided not to use that with the llvm.fpbuiltin intrinsics because of the potential loss of semantics if the metadata is dropped. This is only really acceptable for operations that default to correctly rounded implementations.

asudarsa · 2023-06-21T15:47:40Z

I think we've talked about this, but I can't remember the reasoning. Can you tell me why we need to translate this to metadata and not an attribute at the call site when translating from SPIR-V back to LLVM IR? The metadata is subject to being dropped, which would change the semantics of the call.

If we must use metadata, the standard "fpmath" metadata has the same meaning. We decided not to use that with the llvm.fpbuiltin intrinsics because of the potential loss of semantics if the metadata is dropped. This is only really acceptable for operations that default to correctly rounded implementations.

Hi @vmustya

Can you please provide your feedback here? I remember you mentioned that we use metadata here (instead of attributes) during the 'spec' discussions. Can you please let us know if metadata can be replaced by attributes here?

Thanks

vmustya · 2023-06-21T15:54:06Z

Hi @vmustya

Can you please provide your feedback here? I remember you mentioned that we use metadata here (instead of attributes) during the 'spec' discussions. Can you please let us know if metadata can be replaced by attributes here?

Thanks

The attributes look good enough to me. I've only voted against non-standard custom intrinsics.

asudarsa · 2023-06-21T17:32:49Z

I think we've talked about this, but I can't remember the reasoning. Can you tell me why we need to translate this to metadata and not an attribute at the call site when translating from SPIR-V back to LLVM IR? The metadata is subject to being dropped, which would change the semantics of the call.

If we must use metadata, the standard "fpmath" metadata has the same meaning. We decided not to use that with the llvm.fpbuiltin intrinsics because of the potential loss of semantics if the metadata is dropped. This is only really acceptable for operations that default to correctly rounded implementations.

Adding @GarveyJoe and @shuoniu-intel for more comments on this. Thanks

andykaylor · 2023-06-21T17:42:03Z

Since Victor is OK with using an attribute, I definitely think that's the right thing to do. We can't guarantee that metadata would not be lost before it is needed.

MrSidims · 2023-07-03T09:47:42Z

I have several high level questions to start with.
0. The intrinsics do not exist in llvm.org. Do we really want this patch present here and not in intel/llvm where these intrinsics are declared? If we want the patch here, shouldn't we guard their translation by an option like it's done for genx intrinsics? Can discuss it during WG call.

Are these intrinsics coming from some high-level user/library API or from optimizations? If it's coming only from high-level APIs shouldn't we explore SPIR-V friendly LLVM IR mechanism capabilities for this feature, see the following paragraphs: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#id16 and https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#id9 , using it we can get rid of the intrinsics completely in case if we don't have plans to upstream or llvm community doesn't want them. If the intrinsics can come from transformations, can't we live with just attribute/metadata in the input to SPIR-V translator IR if we generate this output anyway?
What happens if the attribute/metadata came attached to ocl_sin builtin call? Shouldn't we handle it as well?
We can't guarantee that metadata would not be lost before it is needed.
The metadata is applied to external function call. It's hard to imagine losing it unless we link two modules with the following inlining (which I assume can happen later in some BE). But won't attribute be lost in this case as well?

asudarsa · 2023-07-06T16:45:46Z

Hi @MrSidims
Thanks much for the feedback.
Please find answers inlined below.

I have several high level questions to start with. 0. The intrinsics do not exist in llvm.org. Do we really want this patch present here and not in intel/llvm where these intrinsics are declared? If we want the patch here, shouldn't we guard their translation by an option like it's done for genx intrinsics? Can discuss it during WG call.
ANSWER: This is a good suggestion. One point to note. This PR does not require the intrinsics to exist in llvm.org. We rely on using string comparisons here to check for called builtin function names. So, these changes do work with llvm.org. But, I do not have any objections to move it to intel/llvm and cherry-pick to khronos once the intrinsics are available in llvm.org.

Are these intrinsics coming from some high-level user/library API or from optimizations? If it's coming only from high-level APIs shouldn't we explore SPIR-V friendly LLVM IR mechanism capabilities for this feature, see the following paragraphs: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#id16 and https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#id9 , using it we can get rid of the intrinsics completely in case if we don't have plans to upstream or llvm community doesn't want them. If the intrinsics can come from transformations, can't we live with just attribute/metadata in the input to SPIR-V translator IR if we generate this output anyway?
ANSWER: I think these builtins are generated by compiler. Please see Add new intrinsics and attributes to control accuracy of FP calls intel/llvm#8134
I think @andykaylor may be able to answer this better.

What happens if the attribute/metadata came attached to ocl_sin builtin call? Shouldn't we handle it as well?
ANSWER: Can you please provide an example, if possible? I did not understand this case. Sorry.

We can't guarantee that metadata would not be lost before it is needed.
The metadata is applied to external function call. It's hard to imagine losing it unless we link two modules with the following inlining (which I assume can happen later in some BE). But won't attribute be lost in this case as well?
ANSWER: Base on my experiments, I think adding metadata is a better option as we do end up non-function FP operations in some cases and we cannot generate attributes for them.

asudarsa · 2023-07-07T16:50:33Z

lib/SPIRV/SPIRVWriter.cpp

+                                         SPIRVInstruction *I) {
+  const bool AllowFPMaxError =
+      BM->isAllowedToUseExtension(ExtensionID::SPV_INTEL_fp_max_error);
+  bool IsLLVMFPBuiltin =


llvm.fpbuiltin.* is a set of new LLVM builtins introduced in the open-source SYCL compiler (intel/llvm#8134). It is not yet upstreamed to LLVM.org compiler.
Support to translate these builtins currently rely on matching the name of the called function (as done here).
This matching will be replaced with matching with the actual Intrinsic ID if and when the front-end compiler change is available in LLVM.org.

@svenvh,

Can you please take a look at this and comment if this is an agreeable solution?

Thanks

Perhaps it's worth starting an RFC thread on discourse.llvm.org first, to see if the intrinsics have a chance of getting accepted into llvm main.

Hi @svenvh

Thanks for comment. There is already a thread open here: https://discourse.llvm.org/t/rfc-floating-point-accuracy-control/66018. Discussion is ongoing.

Thanks for pointing to the thread. I've skimmed over it, but it seems there hasn't been strong consensus yet and the discussion seems to have stalled. With that in mind, I'd probably prefer to avoid the llvm. prefix for the new "intrinsics", as they aren't LLVM intrinsics really.

Hi @svenvh

Thanks for looking at this. The builtin naming is actually coming from intel/llvm#8134 and we expect this to get added to llorg at some point of time. The current PR does not have control over the naming.

…asic math functions (add/sub/mul/div/rem) Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

…ror_support

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

…ror_support

asudarsa · 2023-07-26T15:10:19Z

Hi @svenvh and @MrSidims

SPIR-V headers and SPIR-V tools changes have been merged. We are now waiting on this PR. Can you please take a look when convenient?

Thanks
Sincerely

MrSidims

Overall LGTM

lib/SPIRV/SPIRVReader.cpp

MrSidims · 2023-07-27T11:31:59Z

lib/SPIRV/SPIRVWriter.cpp

+      break;
+    SPIRVType *STy = transType(II->getType());
+    std::vector<SPIRVValue *> Ops(1, transValue(II->getArgOperand(0), BB));
+    auto ExtOp = StringSwitch<SPIRVWord>(OpName)


Does it suppose to work for ESIMD SYCL programming model? I'm asking because not all backends support OpenCL ext instructions

We do expect the backends to atleast support the subset of instructions that are shown here.

Thanks

@vmustya just checking your opinion

Currently IGC VC backend only supports the native_* subset of OpenCL extended instructions.

@asudarsa to consider changing math ext instructions to native_*

@andykaylor I'm talking about OpenCL builtins described here: https://registry.khronos.org/SPIR-V/specs/1.0/OpenCL.ExtendedInstructionSet.100.mobile.html native vs non-native. AFAIK IGC scalar support all of the builtins, while vector compiler support only 'native'. I just want to ensure, that we are on the same page and understand consequences of merging implementation going through non-native builtins.

Performance-wise: that's what the spec says:
The function may map to one or more native device instructions and will typically have better performance compared to the non native corresponding functions. Support for denormal values is implementation-defined for this function
I can neither confirm nor deny such statement for Intel and others hardware.

@MrSidims My concern is for the case where we're trying to restrict accuracy beyond the normal SYCL requirements. For example, the cos() function normally only requires 4 ulp accuracy, but I might want to call it with a 1 ulp accuracy requirement. My understanding of the native_ OCL instructions is that native instructions may be used regardless of their accuracy. So if we're trying to require 1 ulp accuracy, using the native_ instructions isn't appropriate.

@andykaylor thanks for the explanation! I just wanted to ensure, that we understand that we sacrifice portability (at least temporary) and have a reasoning for it.
@asudarsa please resolve the conflict and I'll merge the PR.

@MrSidims Yes, sacrificing portability when the accuracy controls are used is expected. I expect that the accuracy controls will only be used by advanced users who are trying to fine-tune their implementations. I hope that if the feature is successful more vendors will add support for it and the portability problem will be resolved.

lib/SPIRV/SPIRVWriter.cpp

test/extensions/INTEL/SPV_INTEL_fp_max_error/IntelFPMaxError.ll

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

MrSidims

LGTM
Would be nice if @vmustya @vmaksimo @jgstarIntel @LU-JOHN and @maksimsab also took a look

vmaksimo

LGTM, just one comment

vmaksimo · 2023-07-31T16:39:08Z

lib/SPIRV/SPIRVReader.cpp

+  SPIRVWord ID;
+  if (Instruction *I = dyn_cast<Instruction>(V))
+    if (BV->hasDecorate(DecorationFPMaxErrorDecorationINTEL, 0, &ID)) {
+      auto Literals =
+          BV->getDecorationLiterals(DecorationFPMaxErrorDecorationINTEL);
+      assert(Literals.size() == 1 &&
+             "FP Max Error decoration shall have 1 operand");
+      auto F = convertSPIRVWordToFloat(Literals[0]);
+      if (CallInst *CI = dyn_cast<CallInst>(I)) {
+        // Add attribute
+        auto A = llvm::Attribute::get(*Context, "fpbuiltin-max-error",
+                                      std::to_string(F));
+        CI->addFnAttr(A);
+      } else {
+        // Add metadata
+        MDNode *N =
+            MDNode::get(*Context, MDString::get(*Context, std::to_string(F)));
+        I->setMetadata("fpbuiltin-max-error", N);
+      }
+      return true;
+    }


Just minor suggestion to wrap this to a separate function - to have more consistent code in transDecoration()

Signed-off-by: Sudarsanam, Arvind <arvind.sudarsanam@intel.com>

asudarsa · 2023-08-31T20:30:00Z

There is only a minor formatting issue which I will fix just before final merge. I think we can proceed with reviews/approvals.

Thanks

Signed-off-by: Sudarsanam, Arvind <arvind.sudarsanam@intel.com>

asudarsa added 5 commits June 16, 2023 15:40

Add support for SPV_INTEL_fp_max_error extension

3564ad5

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Merge remote-tracking branch 'real-origin/main' into asudarsa/add_sup…

8d9ef15

…port_for_SPV_INTEL_fp_max_error_spec_extension

Merge remote-tracking branch 'real-origin/main' into asudarsa/add_sup…

13df725

…port_for_SPV_INTEL_fp_max_error_spec_extension

Test changes

132c531

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

formatting

e1d0775

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa marked this pull request as draft June 19, 2023 01:44

asudarsa commented Jul 7, 2023

View reviewed changes

asudarsa added 6 commits July 13, 2023 15:51

Add attributes for some math builtin functions and add metadata for b…

340d530

…asic math functions (add/sub/mul/div/rem) Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Merge remote-tracking branch 'real-origin/main' into fpbuiltin_max_er…

66f1cd6

…ror_support

Update SPIRV-Headers tag

20bd965

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

fix clang-tidy issues

22f367b

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

fix clang-format changes

c20e426

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

update spirv headers tag

1f7a4ab

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa marked this pull request as ready for review July 22, 2023 00:55

Merge remote-tracking branch 'real-origin/main' into fpbuiltin_max_er…

5df3983

…ror_support

asudarsa requested a review from svenvh July 26, 2023 15:14

MrSidims reviewed Jul 27, 2023

View reviewed changes

asudarsa added 2 commits July 28, 2023 18:23

Address review comments

bc3eeac

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Address more review comments

eaa8573

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa requested a review from MrSidims July 29, 2023 01:27

MrSidims approved these changes Jul 31, 2023

View reviewed changes

vmaksimo reviewed Jul 31, 2023

View reviewed changes

asudarsa added 3 commits August 31, 2023 06:37

Merge branch 'main' into fpbuiltin_max_error_support

b4c5b7a

Update test

1303aa5

Signed-off-by: Sudarsanam, Arvind <arvind.sudarsanam@intel.com>

Address one of the review comments

95f0190

Signed-off-by: Sudarsanam, Arvind <arvind.sudarsanam@intel.com>

asudarsa requested a review from MrSidims August 31, 2023 20:28

MrSidims approved these changes Sep 1, 2023

View reviewed changes

asudarsa added 2 commits September 5, 2023 06:23

Merge branch 'main' into fpbuiltin_max_error_support

833bd78

Fix clang-format issue

2f40dbf

Signed-off-by: Sudarsanam, Arvind <arvind.sudarsanam@intel.com>

MrSidims merged commit c6fe12b into KhronosGroup:main Sep 5, 2023
8 checks passed

asudarsa mentioned this pull request Oct 24, 2023

[Builtin] Fix issue with attribute list update #2192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPIR-V Extension] fpbuiltin-max-error support #2056

[SPIR-V Extension] fpbuiltin-max-error support #2056

asudarsa commented Jun 19, 2023 •

edited

Loading

asudarsa commented Jun 19, 2023

asudarsa commented Jun 19, 2023

andykaylor commented Jun 20, 2023

asudarsa commented Jun 21, 2023

vmustya commented Jun 21, 2023

asudarsa commented Jun 21, 2023

andykaylor commented Jun 21, 2023

MrSidims commented Jul 3, 2023 •

edited

Loading

asudarsa commented Jul 6, 2023

asudarsa Jul 7, 2023

svenvh Jul 7, 2023

asudarsa Jul 10, 2023

svenvh Jul 10, 2023

asudarsa Jul 10, 2023

asudarsa commented Jul 26, 2023

MrSidims left a comment

MrSidims Jul 27, 2023 •

edited

Loading

asudarsa Jul 29, 2023

MrSidims Jul 31, 2023

vmustya Jul 31, 2023

MrSidims Aug 28, 2023

MrSidims Aug 29, 2023

MrSidims Aug 29, 2023 •

edited

Loading

andykaylor Aug 29, 2023

MrSidims Aug 29, 2023

andykaylor Aug 29, 2023

MrSidims left a comment

vmaksimo left a comment

vmaksimo Jul 31, 2023

asudarsa commented Aug 31, 2023

[SPIR-V Extension] fpbuiltin-max-error support #2056

[SPIR-V Extension] fpbuiltin-max-error support #2056

Conversation

asudarsa commented Jun 19, 2023 • edited Loading

asudarsa commented Jun 19, 2023

asudarsa commented Jun 19, 2023

andykaylor commented Jun 20, 2023

asudarsa commented Jun 21, 2023

vmustya commented Jun 21, 2023

asudarsa commented Jun 21, 2023

andykaylor commented Jun 21, 2023

MrSidims commented Jul 3, 2023 • edited Loading

asudarsa commented Jul 6, 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

Choose a reason for hiding this comment

asudarsa commented Jul 26, 2023

MrSidims left a comment

Choose a reason for hiding this comment

MrSidims Jul 27, 2023 • 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

MrSidims Aug 29, 2023 • 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

MrSidims left a comment

Choose a reason for hiding this comment

vmaksimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudarsa commented Aug 31, 2023

asudarsa commented Jun 19, 2023 •

edited

Loading

MrSidims commented Jul 3, 2023 •

edited

Loading

MrSidims Jul 27, 2023 •

edited

Loading

MrSidims Aug 29, 2023 •

edited

Loading