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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

> I'm accessing the flag on LLVMContextImpl, but possibly the ForceOpaquePointers mode should be part of the public LLVM context API?

I agree that adding something to LLVMContext itself would be cleaner. I suspect this flag will be needed in other places too!

BTW, the proposed API might be more forward-looking / clear if it talked about what's supported rather than what's being forced. Something like inverting to `supportsTypedPointers()` (or using `onlySupportsOpaquePointers()`). Forcing implies a change from a default state. I figure there'll be some period of time where the default will be NOT to support typed pointers, and this flag becomes a way to turn off an on-by-default, until eventually typed pointers are eliminated entirely... at which point maybe the command-line option can/should be renamed or removed, but it'd be nice not to have churn in the LLVMContext API.



================
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;
----------------
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).



================
Comment at: llvm/test/Assembler/remangle-intrinsic-opaque-ptr.ll:1-4
+; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
+
+; An opaque pointer type should not be accepted for an intrinsic that
+; specifies a fixed pointer type, outside of --force-opaque-pointers mode.
----------------
It'd be good to add a positive `RUN:` line (using `--force-opaque-pointers` and a different CHECK prefix) to confirm this testcase doesn't bitrot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105155