MIRParser: Accept overloaded intrinsic names w/o type suffixes

[rtereshin]

Summary

MIRParser: Accept overloaded intrinsic names w/o type suffixes

Function::lookupIntrinsicID is somewhat forgiving as it comes to
overloaded intrinsics' names: it returns an ID as soon as the name
provided has a prefix that matches a registered intrinsic's name w/o
actually checking that the rest of the name encodes all the concrete arg
types, let alone that those types are compatible with the intrinsic's
definition.

That's probably fine and comes in handy in MIR serialization: we don't
care about IR types at MIR level and every intrinsic should be
selectable based on its ID and low-level types (LLTs) of its operands,
including the overloaded ones, so there is no point in serializing
mangled IR types as part of the intrinsic's name.

However, lookupIntrinsicID is somewhat inconsistent in its forgiveness:
if the name provided is actually an exact match, it will refuse to
return the ID if the intrinsic is overloaded. There is probably no
real reason for that and it renders MIRParser incapable to deserialize
MIR MIRPrinter serialized.

This patch fixes this.

[rtereshin]

Ping

[rtereshin]

Ping

lib/IR/Function.cpp

528 ↗

(On Diff #134144)

This would return not_intrinsic if it's an exact match, but the intrinsic happens to be overloaded.

530 ↗

(On Diff #134144)

If it's an exact match, this will return the ID regardless of the intrinsic being overloaded or not.

[bogner]

Looks good, thanks

test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir

1–3 ↗

(On Diff #134144)

It might be easier to tell what went wrong on failures if we output to a temporary file (ie %t) instead of doing this all in a single pipeline.

9–24 ↗

(On Diff #134144)

I guess it makes sense since this is a test for the mir parser, but it's a bit odd to have a mir file that only consists of llvm ir.

[rtereshin]

test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir

1–3 ↗

(On Diff #134144)

Sure! I'm changing the RUN lines to:

# RUN: llc -mtriple aarch64-- -run-pass irtranslator -simplify-mir %s -o %t \
# RUN:   -verify-machineinstrs; llc -mtriple aarch64-- -run-pass legalizer \
# RUN:   -simplify-mir %t -x mir -o - -verify-machineinstrs | FileCheck %s

9–24 ↗

(On Diff #134144)

Apparently, -run-pass command line option isn't defined for IR files, llc fails like follows:

llc: run-pass is for .mir file only.

as discussed offline.

We should probably change that at some point, as well as make it possible to run IR-to-IR passes that are ran by llc anyway separately, in opt manner. Not to encourage people adding more IR-to-IR passes into llc's pipeline, but to make it easier to test and troubleshoot.

llvm/trunk/lib/IR/Function.cpp

if (Idx == -1)
     return Intrinsic::not_intrinsic;
 
   // Intrinsic IDs correspond to the location in IntrinsicNameTable, but we have
   // an index into a sub-table.
   int Adjust = NameTable.data() - IntrinsicNameTable;
   Intrinsic::ID ID = static_cast<Intrinsic::ID>(Idx + Adjust);
 
   // If the intrinsic is not overloaded, require an exact match. If it is
-  // overloaded, require a prefix match.
-  bool IsPrefixMatch = Name.size() > strlen(NameTable[Idx]);
-  return IsPrefixMatch == isOverloaded(ID) ? ID : Intrinsic::not_intrinsic;
+  // overloaded, require either exact or prefix match.
+  const auto MatchSize = strlen(NameTable[Idx]);
+  assert(Name.size() >= MatchSize && "Expected either exact or prefix match");
+  bool IsExactMatch = Name.size() == MatchSize;
+  return IsExactMatch || isOverloaded(ID) ? ID : Intrinsic::not_intrinsic;
 }
 
 void Function::recalculateIntrinsicID() {
   StringRef Name = getName();
   if (!Name.startswith("llvm.")) {
     HasLLVMReservedName = false;
     IntID = Intrinsic::not_intrinsic;
     return;

llvm/trunk/test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir

+# RUN: llc -mtriple aarch64-- -run-pass irtranslator -simplify-mir %s -o %t \
+# RUN:   -verify-machineinstrs; llc -mtriple aarch64-- -run-pass legalizer \
+# RUN:   -simplify-mir %t -x mir -o - -verify-machineinstrs | FileCheck %s
+
+# Test that MIRParser is able to deserialize back MIR MIRPrinter serialized,
+# specifically overloaded intrinsic names in this case which aren't required
+# to encode all the concrete arg types in the name at MIR level.
+
+--- |
+  define i32 @int_aarch64_sdiv(i32 %a, i32 %b) nounwind readnone ssp {
+  ; CHECK-LABEL: name: int_aarch64_sdiv
+  ; CHECK: liveins: $w0, $w1
+  ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
+  ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $w1
+  ; CHECK: [[INT:%[0-9]+]]:_(s32) = G_INTRINSIC intrinsic(@llvm.aarch64.sdiv), [[COPY]](s32), [[COPY1]](s32)
+  ; CHECK: $w0 = COPY [[INT]](s32)
+  ; CHECK: RET_ReallyLR implicit $w0
+  entry:
+    %sdiv = call i32 @llvm.aarch64.sdiv.i32(i32 %a, i32 %b)
+    ret i32 %sdiv
+  }
+
+  declare i32 @llvm.aarch64.sdiv.i32(i32, i32) nounwind readnone
+...