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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks 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;
----------------
nikic wrote:
> dexonsmith wrote:
> > nikic wrote:
> > > 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.
> > On 1: agreed that this sounds like excessive churn.
> > 
> > On 2: I see; and updating code to see look through the bitcast would also be a ton of churn that would eventually just be deleted.
> > 
> > Stepping back, I wonder if it'd be useful to transition such intrinsics over to using opaque pointers instead of `i8*`, as a way of breaking out a manageable chunk of the overall transition (reducing the impact of `--force-opaque-pointers`). For example, intrinsics could be moved incrementally one-at-time or in logical groups (e.g., change `@llvm.va_start` and `@llvm.va_end` to use `ptr` instead of `i8*`). Seems like any code changes for this would need to be done anyway, and probably aren't that hard... and this way there's less code multiplexing between the two modes.
> > 
> > Not suggesting you do that instead of this patch, more of a general suggestion that could be done separately... although if this implementation makes it illegal for an intrinsic to choose `ptr` its fixed pointer type outside of `--force-opaque-pointers`, that'd have to massaged somehow...
> The core problem with doing that is that as soon as we generate opaque pointers somewhere, these might propagate into some place that doesn't yet support them, so that's something we could only do relatively late-stage in the migration with confidence. So I don't think that's a useful consideration at this point in time.
> 
> I did play with the idea of having something like a "convert to opaque pointers" pass that could be inserted early in the pipeline to allow more exposure to opaque pointers by converting things like function arguments. Ultimately, I'm not sure how useful something like that would be vs a full switch, because the mixed mode differs significantly from the fully opaque one. It might be useful if clang continues to lag behind badly.
I never intended for `--force-opaque-pointers` to be used for anything other than checking if we're good to go to update all tests. The plan was to turn on --force-opaque-pointers locally, run check-llvm, check-clang, build a large codebase with clang, etc., and see if there are crashes. Once we think we're all good, we start updating llvm tests to use opaque pointers. After that, remove --force-opaque-pointers.

My opinion is that we should go with #2 and expect either a declaration of the intrinsic with a `ptr` return type, or a declaration with an `i8*` return type, but not both.

I don't think supporting mixed opaque pointer and non-opaque pointer modules is something we explicitly want to support. It may happen to work in some (maybe even many) cases, but in cases with intrinsics like this it won't really make sense to. And we definitely don't want to support the same intrinsic twice with different return types in the same module.


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

https://reviews.llvm.org/D105155