[SPIRV] Handle unknown intrinsics

[AlexVlx]

This ports rather useful functionality that was already available in the Translator, and was mostly implemented in the BE. Today, if we encounter an unknown intrinsic, we pipe it through and hope for the best, which in practice yields either obtuse ISEL errors, or potentially impossible to use SPIR-V. With this change, if instructed via a newly added --spv-allow-unknown-intrinsics flag, we emit allowed intrinsics as calls to extern (import) functions. The test is also mostly lifted from the Translator.

[llvmbot]

@llvm/pr-subscribers-backend-spir-v

Author: Alex Voicu (AlexVlx)

Changes

This ports rather useful functionality that was already available in the Translator, and was mostly implemented in the BE. Today, if we encounter an unknown intrinsic, we pipe it through and hope for the best, which in practice yields either obtuse ISEL errors, or potentially impossible to use SPIR-V. With this change, if instructed via a newly added --spv-allow-unknown-intrinsics flag, we emit allowed intrinsics as calls to extern (import) functions. The test is also mostly lifted from the Translator.

---

Full diff: https://github.com/llvm/llvm-project/pull/166284.diff

2 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp (+21)
• (added) llvm/test/CodeGen/SPIRV/allow_unknown_intrinsics.ll (+38)

diff --git a/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp b/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
index 4e4e6fb4ab791..3334bb18deab9 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
@@ -56,6 +56,13 @@ class SPIRVPrepareFunctions : public ModulePass {
   }
 };
 
+static cl::list<std::string> SPVAllowUnknownIntrinsics(
+    "spv-allow-unknown-intrinsics", cl::CommaSeparated,
+    cl::desc("Emit unknown intrinsics as calls to external functions. If a "
+             "comma-separated input list of intrinsic prefixes is provided, "
+             "only intrinsics carrying a listed prefix get emitted. Otherwise, "
+             "all unknown intrinsics are emitted"),
+    cl::value_desc("intrinsic_prefix_0,intrinsic_prefix_1"), cl::ValueOptional);
 } // namespace
 
 char SPIRVPrepareFunctions::ID = 0;
@@ -445,6 +452,20 @@ bool SPIRVPrepareFunctions::substituteIntrinsicCalls(Function *F) {
                                        EraseFromParent);
         Changed = true;
         break;
+      default:
+        if (TM.getTargetTriple().getVendor() == Triple::AMD ||
+            any_of(SPVAllowUnknownIntrinsics, [II](auto &&Prefix) {
+              return II->getCalledFunction()->getName().starts_with(Prefix);
+            }))
+          Changed |= lowerIntrinsicToFunction(II);
+        else
+          report_fatal_error(
+              "Encountered unknown intrinsic: " +
+                II->getCalledFunction()->getName() +
+                ", which requires the --spv-allow-unknown-intrinsics option, "
+                "in function: " + II->getParent()->getParent()->getName(),
+              false);
+        break;
       }
     }
   }
diff --git a/llvm/test/CodeGen/SPIRV/allow_unknown_intrinsics.ll b/llvm/test/CodeGen/SPIRV/allow_unknown_intrinsics.ll
new file mode 100644
index 0000000000000..872c4a68a8160
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/allow_unknown_intrinsics.ll
@@ -0,0 +1,38 @@
+; RUN: not llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o %t.spvt 2>&1 | FileCheck -check-prefix=CHECK-ERROR %s
+; RUN: not llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics=notllvm %s -o %t.spvt 2>&1 | FileCheck --check-prefix=CHECK-ERROR %s
+; RUN: not llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics=llvm.some.custom %s -o %t.spvt 2>&1 | FileCheck --check-prefix=CHECK-ERROR %s
+; RUN: not llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics=llvm.readcyclecounter %s -o %t.spvt 2>&1 | FileCheck --check-prefix=CHECK-ERROR1 %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics=llvm. %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics=llvm.,random.prefix %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics=llvm %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-amd-amdhsa %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown --spv-allow-unknown-intrinsics %s -o - -filetype=obj | spirv-val %}
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-amd-amdhsa %s -o - -filetype=obj | spirv-val %}
+
+; The test checks command-line option which allows to represent unknown
+; intrinsics as external function calls in SPIR-V.
+
+; CHECK-ERROR: LLVM ERROR: Encountered unknown intrinsic: llvm.readcyclecounter, which requires the --spv-allow-unknown-intrinsics option, in function: foo
+; CHECK-ERROR1: LLVM ERROR: Encountered unknown intrinsic: llvm.some.custom.intrinsic, which requires the --spv-allow-unknown-intrinsics option, in function: foo
+
+; CHECK: Name %[[READCYCLECOUNTER:[0-9]+]] "spirv.llvm_readcyclecounter"
+; CHECK: Name %[[SOME_CUSTOM_INTRINSIC:[0-9]+]] "spirv.llvm_some_custom_intrinsic"
+; CHECK-DAG: Decorate %[[READCYCLECOUNTER]] LinkageAttributes {{.*}} Import
+; CHECK: Decorate %[[SOME_CUSTOM_INTRINSIC]] LinkageAttributes {{.*}} Import
+; CHECK-DAG: %[[I64:[0-9]+]] = OpTypeInt 64
+; CHECK: %[[FnTy:[0-9]+]] = OpTypeFunction %[[I64]]
+; CHECK: %[[READCYCLECOUNTER]] = OpFunction %[[I64]] {{.*}} %[[FnTy]]
+; CHECK-DAG: %[[SOME_CUSTOM_INTRINSIC]] = OpFunction %[[I64]] {{.*}} %[[FnTy]]
+; CHECK-DAG: OpFunctionCall %[[I64]] %[[READCYCLECOUNTER]]
+; CHECK:     OpFunctionCall %[[I64]] %[[SOME_CUSTOM_INTRINSIC]]
+
+define spir_func void @foo() {
+entry:
+  %0 = call i64 @llvm.readcyclecounter()
+  %1 = call i64 @llvm.some.custom.intrinsic()
+  ret void
+}
+
+declare i64 @llvm.readcyclecounter()
+declare i64 @llvm.some.custom.intrinsic()

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[AlexVlx]

Turns out that this cannot quite work as envisioned due to a lack of a clear delineation between intrinsics that work and intrinsics that do not work, so I am going to change it a bit and remove the failure case.

[MrSidims]

TBH I'm not the greatest fan of this llvm-spirv option. Usually when compilation comes to this point the intrinsics should be resolved by the SPIR-V generator. Also even if the intention is to have them translated back to LLVM IR it opens some uncomfortable questions regarding change of the semantics or intrinsics' postfixes for overloads (for example switch from typed to untyped pointers made some intrinsics incompatible (at least out of a box)).

On the other hand there are vendor specific intrinsics, that might not have a counterpart in the upstream LLVM or that are better not to be emulated.

How do you feel about always requiring a Prefix for --spv-allow-unknown-intrinsics? Like this we could at least create a notion, that 'native' LLVM intrinsics should be lowered to SPIR-V appropriately?

[AlexVlx]

TBH I'm not the greatest fan of this llvm-spirv option. Usually when compilation comes to this point the intrinsics should be resolved by the SPIR-V generator. Also even if the intention is to have them translated back to LLVM IR it opens some uncomfortable questions regarding change of the semantics or intrinsics' postfixes for overloads (for example switch from typed to untyped pointers made some intrinsics incompatible (at least out of a box)).

On the other hand there are vendor specific intrinsics, that might not have a counterpart in the upstream LLVM or that are better not to be emulated.

How do you feel about always requiring a Prefix for --spv-allow-unknown-intrinsics? Like this we could at least create a notion, that 'native' LLVM intrinsics should be lowered to SPIR-V appropriately?

I agree overall, it's not ideal, but as you correctly point out, it's extremely unlikely that we'll ever be in a position to handle useful target specific intrinsics in a more meaningful way. Always requiring a prefix seems reasonable, however it would probably be aspirational at this point (as the test itself outlines, we have gaps around fairly boring things like readcyclecounter). That would still be fine though as the user could explicitly specify llvm. as the prefix and still make progress, being able to do away with the option / constrain it to only match target specific intrinsics. I assume this is what you had in mind, and not outright banning ever handling native LLVM intrinsics via this mechanism.

[MrSidims]

the user could explicitly specify llvm.
I assume this is what you had in mind, and not outright banning ever handling native LLVM intrinsics via this mechanism.

Yes, I though that we can say: lets ban a stand-alone llvm prefix, but it might be a bit odd. So the idea is to 'create a notion' that native intrinsics should be lowered by backend and not blindly translated to a function call :)

[AlexVlx]

the user could explicitly specify llvm.
I assume this is what you had in mind, and not outright banning ever handling native LLVM intrinsics via this mechanism.

Yes, I though that we can say: lets ban a stand-alone llvm prefix, but it might be a bit odd. So the idea is to 'create a notion' that native intrinsics should be lowered by backend and not blindly translated to a function call :)

Done-ish, if you'd like to take another peek. I considered adding a warning for the case where the user just provides the option with no prefixes, but that seemed to entail a bit of needless ritual / increased the footprint. It would have been nice to use ValueRequired, but that behaves surprisingly / doesn't allow the = syntax. We should probably revisit this in the future.

[MrSidims]

LGTM, but would prefer to hear from somebody else in the reviewers list before merge

[Keenuts]

Hello!
What's the end goal here? Seems like you want to allow any intrinsic to be lowered to external calls when targeting AMDGPU. Do you plan to this as a valid flow?
Is the goal to compile something -> SPIR-V + external to then simplify lifting back the SPIR-V to LLVM IR with proper intrinsics to feed it in a driver or something?

[AlexVlx]

Hello! What's the end goal here? Seems like you want to allow any intrinsic to be lowered to external calls when targeting AMDGPU. Do you plan to this as a valid flow? Is the goal to compile something -> SPIR-V + external to then simplify lifting back the SPIR-V to LLVM IR with proper intrinsics to feed it in a driver or something?

Very good questions, thank you! This is already the flow we have in place / use, via the translator. The core benefit is the retention of target specific intrinsics, which then get consumed by the target BE which knows what to do with them. Orthogonally to AMDGPU (as this is not gated on AMDGPU), it does give a side-channel for handling intrinsics that are known to GISEL, but aren't currently handled by the BE - the test illustrates a case of this. As @MrSidims pointed out above, we'll eventually want to have valid SPIR-V lowerings for all vanilla LLVM intrinsics, but until we get there this would allow targets to make forward progress / extant code using builtins or intrinsics directly (ick) to just work with SPIR-V, rather than having to refactor. Hopefully that makes some amount of sense.

[Keenuts]

Right, thanks for the comment. So if I understand, if tomorrow I implement a proper lowering for llvm.readcyclecounter() in SPIR-V, this won't break you, just go through another path right?

I think this should be OK, but could you add a comment in the test to explain that if someone implements proper lowering to llvm.readcyclecounter, then they should just replace the intrinsic with another unsupported one in the test as this isn't breaking somebody?

[AlexVlx]

Right, thanks for the comment. So if I understand, if tomorrow I implement a proper lowering for llvm.readcyclecounter() in SPIR-V, this won't break you, just go through another path right?

Yes, this is correct.

I think this should be OK, but could you add a comment in the test to explain that if someone implements proper lowering to llvm.readcyclecounter, then they should just replace the intrinsic with another unsupported one in the test as this isn't breaking somebody?

Will do, thank you.