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

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

dexonsmith accepted this revision.
dexonsmith added a comment.

This LGTM, once you add the `RUN` line for llvm-dis I suggested in the last comment. It's up to you whether to split out the changes for moving the LLVMContext construction after command-line parsing into a prep commit... there could be an argument for keeping it together as well.



================
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:
> 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 :/.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503