[PATCH] D105155: [OpaquePtr] Support opaque pointers in intrinsic type check

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/IR/Function.cpp:1413-1418
+      // When not using --force-opaque-pointers, do not allow using opaque
+      // pointer in place of fixed pointer type. This would make the intrinsic
+      // signature non-unique.
+      LLVMContextImpl *CImpl = Ty->getContext().pImpl;
+      if (!CImpl->ForceOpaquePointers)
+        return true;
----------------
dexonsmith wrote:
> Your approach might be the right / pragmatic way forward, but wanted to check whether the following alternatives had been considered/discussed somewhere for when `--force-opaque-pointers=false`:
> 
> 1. Change mangling for intrinsics to differentiate "opaque pointer" vs other pointers.
> 2. Allow either signature, but require all pointers to be the same as each other within an intrinsic (all `i8*` or all `ptr`), and require an intrinsic to be used consistently across a module (whatever client / use-site adds the declaration to the module is "correct", everyone else needs to bitcast).
> 
For 1: We do already mangle opaque pointers differently. However, this only applies to cases where multiple pointer types are accepted. This already works, and we have a couple tests for stuff like `@llvm.memset.p0(ptr)`. Here the pointer type is fixed, so it's not part of the mangled name. We would have to change the name mangling to include a typed/opaque indicator where none exists right now, This would be a lot of churn for functionality that is useful neither before nor after the opaque pointer migration.

For 2: I'm pretty sure this is going to crash and burn, because a lot of code expects that an intrinsic is actually a Function, not a ConstantExpr.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105155/new/

https://reviews.llvm.org/D105155