RFC: Disallow intrinsics with name that share prefix with an overloaded intrinsic

IR & Optimizations
1

Hi all,

It seems LLVM allows defining a non-overloaded intrinsic where the name has a prefix that matches the name of an overloaded intrinsic. In some cases, that could lead to incorrect intrinsic name lookup and causing the intrinsic to be considered an unknown intrinsic when ideally it shouldn’t.

The patch below demonstrates the issue:

diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 0a74a217a5f0..4de6bdaa45bd 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -2753,6 +2753,9 @@ def int_experimental_convergence_anchor
 def int_experimental_convergence_loop
   : DefaultAttrsIntrinsic<[llvm_token_ty], [], [IntrNoMem, IntrConvergent]>;

+def int_foo : Intrinsic<[llvm_anyint_ty], [llvm_anyint_ty]>;
+def int_foo_i32 : Intrinsic<[llvm_i32_ty], []>;
+
 //===----------------------------------------------------------------------===//
 // Target-specific intrinsics
 //===----------------------------------------------------------------------===//
diff --git a/llvm/unittests/IR/IntrinsicsTest.cpp b/llvm/unittests/IR/IntrinsicsTest.cpp
index 5916a194f76d..10a0f5adec8f 100644
--- a/llvm/unittests/IR/IntrinsicsTest.cpp
+++ b/llvm/unittests/IR/IntrinsicsTest.cpp
@@ -25,6 +25,8 @@
 #include "llvm/IR/IntrinsicsS390.h"
 #include "llvm/IR/IntrinsicsX86.h"
 #include "llvm/IR/Module.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"

 using namespace llvm;
@@ -165,4 +167,16 @@ TEST_F(IntrinsicsTest, InstrProfInheritance) {
   }
 }

+TEST(IntrinsicOverload, Test) {
+  const char *IR = R"(
+    declare i32 @llvm.foo.i16.i16()
+    declare i32 @llvm.foo.i32()
+    declare i32 @llvm.foo.i32.i32()
+  )";
+  LLVMContext Ctx;
+  SMDiagnostic Error;
+  auto Mod = parseAssemblyString(IR, Error, Ctx);
+  for (const Function &F : Mod->getFunctionList())
+   errs() << F.getName() << ": " << F.getIntrinsicID() << '\n';
+}
 } // end namespace

Running this test gives the following output:

[----------] 1 test from IntrinsicOverload
[ RUN      ] IntrinsicOverload.Test
llvm.foo.i16.i16: 169
llvm.foo.i32: 170
llvm.foo.i32.i32: 0
[       OK ] IntrinsicOverload.Test (0 ms)
[----------] 1 test from IntrinsicOverload (0 ms total)

Here int_foo_i32 share has same prefix name as int_foo, so when LLVM looks up the name “llvm.foo.i32.i32” it first matches against “llvm.foo.i32” in Function::lookupIntrinsicID, and since llvm.foo.i32 is not overloaded and it’s not an exact match, return Intrinsic::not_intrinsic.

There may be a way to change the lookupIntrinsicID function to instead try to find an overloaded intrinsic that can match the name and return it instead (may be by segregating the lookup table into one for non-overloaded ones and one for overloaded ones). However, it seems that even if that is fixed, having one intrinsic (overloaded or not) share a prefix with another overloaded one can be confusing as its sort of ambiguous which intrinsic it is.

So, I am proposing that we disallow this case, by adding additional checks in the intrinsic emitter backend to detect this. I am not sure if any upstream backends already have this problem in their intrinsics (hopefully not). Note that this will disallow some cases like the same patch but with the overloaded llvm.foo with just one overload type and hence just one type in the mangling suffix. But may be that’s ok as it leads to better clarify (i.e., any llvm.foo.* should be an overload of the llvm.foo intrinsic and won’t match something else inadvertently).

For clarify, the exact check will be that if there is an overloaded intrinsic defined, any other intrinsic with the overload name as a prefix will not be allowed and will result in an error from the intrinsic emitter backend.

Thanks,
Rahul

2

This sounds reasonable to me. I don’t think that this is intentionally allowed, it’s just that nobody has thought to explicitly forbid this previously.

3

Thanks, I’ll work on adding the checks.

4

AMDGPU has some intrinsics whose names only differ in what looks like a type overload suffix, e.g. int_amdgcn_fdot2 and int_amdgcn_fdot2_f16_f16. But I think this is OK because int_amdgcn_fdot2 is not actually overloaded.

5

Yeah, 2 non overloaded intrinsics that share a common prefix is completely ok, or even when one if a full prefix of another as in this case.

6

I started looking at this and turns out even 2 intrinsics with the same name are allowed by the intrinsic emitter backend. Within LLVM, I guess bad things will happen when say parsing, as its name can only be matched to one of them even though the 2 may have different intrinsic IDs assigned to them. And which one seems non-deterministic based on the non-deterministic order in which records get sorted in the emitter. So, I am first implementing a check to detect and flag that as an error here.

7

The PR to check duplicate names is now committed. I attempted to implement the prefix check as discussed above, but it seems there are existing LLVM intrinsics that violate the check. Here’s a snippet of the warnings produced:

llvm/include/llvm/IR/Intrinsics.td:1631:5: warning: Intrinsic `llvm.experimental.patchpoint.void` cannot share prefix `llvm.experimental.patchpoint` with an overloaded intrinsic
def int_experimental_patchpoint_void : Intrinsic<[],
    ^
llvm/include/llvm/IR/Intrinsics.td:1638:5: warning: Overloaded intrinsic `llvm.experimental.patchpoint` defined here
def int_experimental_patchpoint : Intrinsic<[llvm_any_ty],
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2767:
llvm/include/llvm/IR/IntrinsicsAArch64.td:3079:7: warning: Intrinsic `llvm.aarch64.sve.cntp.c16` cannot share prefix `llvm.aarch64.sve.cntp` with an overloaded intrinsic
  def int_aarch64_sve_cntp_c16
      ^
Included from llvm/include/llvm/IR/Intrinsics.td:2767:
llvm/include/llvm/IR/IntrinsicsAArch64.td:1887:5: warning: Overloaded intrinsic `llvm.aarch64.sve.cntp` defined here
def int_aarch64_sve_cntp : AdvSIMD_SVE_CNTP_Intrinsic;
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2767:
llvm/include/llvm/IR/IntrinsicsAArch64.td:3082:7: warning: Intrinsic `llvm.aarch64.sve.cntp.c32` cannot share prefix `llvm.aarch64.sve.cntp` with an overloaded intrinsic
  def int_aarch64_sve_cntp_c32
      ^
Included from llvm/include/llvm/IR/Intrinsics.td:2767:
llvm/include/llvm/IR/IntrinsicsAArch64.td:1887:5: warning: Overloaded intrinsic `llvm.aarch64.sve.cntp` defined here
def int_aarch64_sve_cntp : AdvSIMD_SVE_CNTP_Intrinsic;

Included from llvm/include/llvm/IR/Intrinsics.td:2772:
llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1704:5: warning: Intrinsic `llvm.amdgcn.struct.buffer.load.lds` cannot share prefix `llvm.amdgcn.struct.buffer.load` with an overloaded intrinsic
def int_amdgcn_struct_buffer_load_lds : AMDGPUStructBufferLoadLDS;
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2772:
llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1204:5: warning: Overloaded intrinsic `llvm.amdgcn.struct.buffer.load` defined here
def int_amdgcn_struct_buffer_load : AMDGPUStructBufferLoad;
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2772:
llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1727:5: warning: Intrinsic `llvm.amdgcn.struct.ptr.buffer.load.lds` cannot share prefix `llvm.amdgcn.struct.ptr.buffer.load` with an overloaded intrinsic
def int_amdgcn_struct_ptr_buffer_load_lds : AMDGPUStructPtrBufferLoadLDS;
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2772:
llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1240:5: warning: Overloaded intrinsic `llvm.amdgcn.struct.ptr.buffer.load` defined heredef int_amdgcn_struct_ptr_buffer_load : AMDGPUStructPtrBufferLoad;
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2772:
llvm/include/llvm/IR/IntrinsicsAMDGPU.td:2264:5: warning: Intrinsic `llvm.amdgcn.wqm.demote` cannot share prefix `llvm.amdgcn.wqm` with an overloaded intrinsic
def int_amdgcn_wqm_demote : Intrinsic<[],
    ^
Included from llvm/include/llvm/IR/Intrinsics.td:2772:
llvm/include/llvm/IR/IntrinsicsAMDGPU.td:2237:5: warning: Overloaded intrinsic `llvm.amdgcn.wqm` defined here
def int_amdgcn_wqm : Intrinsic<[llvm_any_ty],

So, the check cannot be that strict as it would break existing intrinsics. As a result, I changed my prototype to relax the check so as to allow all of these to go through. I have a WIP MR here, but need some feedback before finishing it. Will discuss it further on the MR.

8

While implementing this check, I found one other edge case that we miss today. Here’s another diff to demonstrate:

diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 0a74a217a5f0..0d2814858509 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1851,6 +1851,9 @@ def int_threadlocal_address : DefaultAttrsIntrinsic<[llvm_anyptr_ty], [LLVMMatch
 def int_stepvector : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
                                             [], [IntrNoMem]>;

+def int_aarch64_foo : Intrinsic<[], [llvm_vararg_ty]>;
+def int_aarch65_foo : Intrinsic<[], [llvm_vararg_ty]>;
+
 //===---------------- Vector Predication Intrinsics --------------===//
 // Memory Intrinsics
 def int_vp_store : DefaultAttrsIntrinsic<[],
@@ -2753,6 +2756,9 @@ def int_experimental_convergence_anchor
 def int_experimental_convergence_loop
   : DefaultAttrsIntrinsic<[llvm_token_ty], [], [IntrNoMem, IntrConvergent]>;

+def int_foo : Intrinsic<[llvm_anyint_ty], [llvm_anyint_ty]>;
+def int_foo_i32 : Intrinsic<[llvm_i32_ty], []>;
+
 //===----------------------------------------------------------------------===//
 // Target-specific intrinsics
 //===----------------------------------------------------------------------===//
diff --git a/llvm/unittests/IR/IntrinsicsTest.cpp b/llvm/unittests/IR/IntrinsicsTest.cpp
index 5916a194f76d..55decd48eac6 100644
--- a/llvm/unittests/IR/IntrinsicsTest.cpp
+++ b/llvm/unittests/IR/IntrinsicsTest.cpp
@@ -25,6 +25,8 @@
 #include "llvm/IR/IntrinsicsS390.h"
 #include "llvm/IR/IntrinsicsX86.h"
 #include "llvm/IR/Module.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"

 using namespace llvm;
@@ -165,4 +167,15 @@ TEST_F(IntrinsicsTest, InstrProfInheritance) {
   }
 }

+TEST(IntrinsicOverload, Test) {
+  const char *IR = R"(
+    declare void @llvm.aarch64.foo(...)
+    declare void @llvm.aarch65.foo(...)
+  )";
+  LLVMContext Ctx;
+  SMDiagnostic Error;
+  auto Mod = parseAssemblyString(IR, Error, Ctx);
+  for (const Function &F : Mod->getFunctionList())
+   errs() << F.getName() << ": " << F.getIntrinsicID() << '\n';
+}
 } // end namespace

Running the test gives:

Note: Google Test filter = *IntrinsicOverload*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from IntrinsicOverload
[ RUN      ] IntrinsicOverload.Test
llvm.aarch64.foo: 0
llvm.aarch65.foo: 2
[       OK ] IntrinsicOverload.Test (0 ms)
[----------] 1 test from IntrinsicOverload (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 1 test.

So the supposedly target independent llvm.aarch64.foo is not correctly matched, because its second dot-separated component matches the name of an existing target, so the findTargetSubtable function in Function.cpp selects aarch64’s table of intrinsics to lookup this intrinsic and fails. So the additional check we need to do in intrinsic emitter is to make sure the second dotted component of a target independent intrinsic name is not equal to one of the defined targets. I’ll now first implement this check and then continue work on the overloaded prefix check.

9

To close on this, this check has now been committed. As a part of this thread, we added 3 new checks:

  1. Check for duplicate intrinsic names.
  2. Check if a target independent intrinsic has name llvm.suffix where suffix is also a target name and disallow that.
  3. This check (for name conflict between overloaded and other intrinsics).