[PATCH] D103503: [OpaquePtr] Introduce option to force all pointers to be opaque pointers

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

It's hard to test the LLVMContext moves without this patch, I'd rather keep it here.



================
Comment at: llvm/lib/IR/Type.cpp:694-695
 PointerType *PointerType::get(Type *EltTy, unsigned AddressSpace) {
+  if (ForceOpaquePointers)
+    return get(EltTy->getContext(), AddressSpace);
+
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > aeubanks wrote:
> > > dexonsmith wrote:
> > > > I wonder if this would be better to thread through as a `bool` set on the `LLVMContext` during construction:
> > > > ```
> > > > if (!EltTy->getContext().supportsTypedPointers())
> > > >   return get(EltTy->getcontext(), AddressSpace);
> > > > ```
> > > > Might make it easier to reason about / access elsewhere. (Could still have the command-line option control that...)
> > > I was worried that looking at a `cl::opt` for every call to `PointerType::get()` could be noticeable perf wise, adding it to LLVMContext(Impl) would mostly fix that.
> > > However, I tried this approach, turns out in many tools, the LLVMContext is created before parsing command line options. I think this flag makes sense as a global flag, not a per-tool flag that requires calling something like `LLVMContext::setForceOpaquePointers()`.
> > > I could go around and fix all the tools to create an LLVMContext after parsing command line options.
> > > I could go around and fix all the tools to create an LLVMContext after parsing command line options.
> > 
> > Maybe this would be the right thing to do, but I'm fine with not going that far for now if you think it's error-prone.
> > 
> > There were really two suggestions above:
> > 
> > 1. Centralize access to the cl::opt via LLVMContext
> > 2. Read the cl::opt only once, during construction of LLVMContext
> > 
> > WDYT of #1 on its own for now? (I.e., use `EltTy->getContext().supportsTypedPointers()` here, but that just reads the `cl::opt`.)
> > 
> > (Personally I'm not too worried about perf of this access/call... my intuition is that the branch would be well-predicted, and there's also a DenseMap lookup to hide behind. Of course I could be wrong though.)
> Looking again, it looks like you've already done this (#1 and #2)... not sure what I was reading before, thinking you hadn't done this yet :/.
Yeah I originally did #1, and now am doing #2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503