[OpaquePtr] Mangle intrinsics with opaque pointers arguments

[zequanwu]

Summary

Mangling intrinsics with opaque pointer arguments using "op"+{address space}.

[aeubanks]

there needs to be tests, and the description should probably state what's going on

[zequanwu]

Add a test case.

[aeubanks]

maybe we should make a quick post on llvm-dev about this to see what other people think, since this will be very user-visible

llvm/lib/IR/Verifier.cpp

1843 ↗

(On Diff #352012)

we still want some of the checks below, for example, we want to check !Attrs.getByValType()->isSized()
perhaps we should separate this out into a separate change

[dblaikie]

(generally looks like the right direction to me - sounds like @aeubanks has some good feedback/issues to address)

[zequanwu]

Rebase.

[aeubanks]

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

llvm/test/Verifier/opaque-ptr.ll

34 ↗

(On Diff #352263)

This is actually not the correct name, it should end with op0. We should add a CHECK line for the actual name.

[zequanwu]

Address comment.

[dblaikie]

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

[zequanwu]

update.

[aeubanks]

In D104272#2820587, @dblaikie wrote:

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

IIUC, we don't really care about the exact name, just that it doesn't conflict with another declaration of the same intrinsic but with different parameter types. This is unambiguous even when non-opaque pointers are in the picture, since non-opaque pointers will always have something after the p0.

[zequanwu]

In D104272#2820587, @dblaikie wrote:

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

It seems what's after "{intrinsic}." is not important for IR parsing: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/IntrinsicInst.cpp#L127.

[aeubanks]

lgtm, now that I understand intrinsics a bit more, this seems trivial enough to not need input from llvm-dev

[dblaikie]

In D104272#2820617, @zequanwu wrote:

In D104272#2820587, @dblaikie wrote:

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

It seems what's after "{intrinsic}." is not important for IR parsing: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/IntrinsicInst.cpp#L127.

Though currently the mangling scheme does allow arbitrary overloading, I think, yeah? If we don't allow arbitrary overloading of intrinsics with the same name, but only use the mangling to fix different parameter types while always having the same number of parameters, then it's probably fine. (then we'd only hit ambiguities in really niche cases maybe?)

But with this change we couldn't overload some intrinsic with "(ptr, int)" and "(ptr)" because they'd produce the same mangling in the case where the former was an opaque pointer and the latter was a typed int pointer?

But we probably don't actually do that?

[aeubanks]

I couldn't parse your comment, could you rephrase?

[dblaikie]

In D104272#2820748, @aeubanks wrote:

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

[zequanwu]

In D104272#2820814, @dblaikie wrote:

In D104272#2820748, @aeubanks wrote:

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

So, it is not ambiguous. Does the patch look good to you?

[dblaikie]

In D104272#2823088, @zequanwu wrote:

In D104272#2820814, @dblaikie wrote:

In D104272#2820748, @aeubanks wrote:

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

So, it is not ambiguous. Does the patch look good to you?

Yep yep, thanks!

llvm/lib/IR/Function.cpp

 /// other function types, we close a function type mangling with suffix 'f'
 /// which can't be confused with it's prefix.  This ensures we don't have
 /// collisions between two unrelated function types. Otherwise, you might
 /// parse ffXX as f(fXX) or f(fX)X.  (X is a placeholder for any other type.)
 /// The HasUnnamedType boolean is set if an unnamed type was encountered,
 /// indicating that extra care must be taken to ensure a unique name.
 static std::string getMangledTypeStr(Type *Ty, bool &HasUnnamedType) {
   std::string Result;
-  if (PointerType* PTyp = dyn_cast<PointerType>(Ty)) {
+  if (PointerType *PTyp = dyn_cast<PointerType>(Ty)) {
+    if (PTyp->isOpaque()) {
+      Result += "op" + utostr(PTyp->getAddressSpace());
+    } else {
       Result += "p" + utostr(PTyp->getAddressSpace()) +
                 getMangledTypeStr(PTyp->getElementType(), HasUnnamedType);
-  } else if (ArrayType* ATyp = dyn_cast<ArrayType>(Ty)) {
+    }
+  } else if (ArrayType *ATyp = dyn_cast<ArrayType>(Ty)) {
     Result += "a" + utostr(ATyp->getNumElements()) +
               getMangledTypeStr(ATyp->getElementType(), HasUnnamedType);
   } else if (StructType *STyp = dyn_cast<StructType>(Ty)) {
     if (!STyp->isLiteral()) {
       Result += "s_";
       if (STyp->hasName())
         Result += STyp->getName();
       else
         HasUnnamedType = true;
     } else {
       Result += "sl_";
       for (auto Elem : STyp->elements())
         Result += getMangledTypeStr(Elem, HasUnnamedType);
     }
     // Ensure nested structs are distinguishable.
     Result += "s";
   } else if (FunctionType *FT = dyn_cast<FunctionType>(Ty)) {
     Result += "f_" + getMangledTypeStr(FT->getReturnType(), HasUnnamedType);
     for (size_t i = 0; i < FT->getNumParams(); i++)
       Result += getMangledTypeStr(FT->getParamType(i), HasUnnamedType);
     if (FT->isVarArg())
       Result += "vararg";
     // Ensure nested function types are distinguishable.
     Result += "f";
-  } else if (VectorType* VTy = dyn_cast<VectorType>(Ty)) {
+  } else if (VectorType *VTy = dyn_cast<VectorType>(Ty)) {
     ElementCount EC = VTy->getElementCount();
     if (EC.isScalable())
       Result += "nx";
     Result += "v" + utostr(EC.getKnownMinValue()) +
               getMangledTypeStr(VTy->getElementType(), HasUnnamedType);
   } else if (Ty) {
     switch (Ty->getTypeID()) {
     default: llvm_unreachable("Unhandled type");