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

Wed Jun 30 14:10:40 PDT 2021

aeubanks accepted this revision.
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:
> > aeubanks wrote:
> > > 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.
> > Yup, you're right, sounds like it'd need to be late stage.
> Currently, `--force-opaque-pointers` is just there to find assertions failure. However, when we actually want to make the switch, I believe that has to happen by flipping the flag, not by converting individual pointer types in tests. Importantly, converting individual types still leaves you with typed pointers for allocas and for globals/functions, because the desired pointer type is never explicitly mentioned for them.
> 
> I don't think that your option #2 can work without some major changes to how we handle intrinsics. We rely on things being unique by design. E.g. if some code creates a call to `Intrinsic::stackprotector`, then it does so using `Intrinsic::getDeclaration()` and expects to get back a function of a certain type. It will generate invalid code if it gets back one with opaque pointer parameters (when we're talking about non-force-opaque-pointers mode) because one happens to be declared in the module already. We also can't just insert a bitcast, because the method returns a `Function*` and changing that to `Constant*`would have some major impact because now it no longer counts as a FunctionCallee.
> 
> I don't think this is something we should try to pursue, especially given how it is only useful for mixed typed and opaque pointer mode. None of this is relevant when working with fully opaque pointers.
Initially I had thought that we'd have separate flags for bitcode/text IR readers to use opaque pointers, but yeah that doesn't address fundamental things like alloca.

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

https://reviews.llvm.org/D105155