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

Thu Jun 24 11:43:24 PDT 2021

dexonsmith added a comment.

(Apologies for disappearing; I was out for a bit, and then lost track of this review.)

================
Comment at: llvm/lib/IR/Type.cpp:694-695
 PointerType *PointerType::get(Type *EltTy, unsigned AddressSpace) {
+  if (ForceOpaquePointers)
+    return get(EltTy->getContext(), AddressSpace);
+
----------------
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.)

================
Comment at: llvm/test/Other/force-opaque-ptrs.ll:1
+; RUN: llvm-as --force-opaque-pointers < %s | llvm-dis | FileCheck %s
+
----------------
aeubanks wrote:
> dblaikie wrote:
> > aeubanks wrote:
> > > dexonsmith wrote:
> > > > Can you explain why this test uses `--force-opaque-pointers`? I feel like this test would be more clear if the textual IR actually matched the IR under test (i.e., if it used `ptr`)... also, then the textual IR won't need to be updated later when typed pointers are removed. But maybe there's some limitation you're working around?
> > > I assumed that at some point we were going to update all tests from `type *` to `ptr`. I think there was a discussion long ago about this but it was never really resolved. We should probably raise this question on llvm-dev.
> > Hmm, I might just end up adding more confusion to this thread, but I'm not sure how your answer, @aeubanks applies to @dexonsmith's question - perhaps you could rephrase?
> > 
> > My understanding of @dexonsmith's question, combined with what @aeubanks has mentioned elsewhere in the review:
> > 
> > `--force-opaque-pointers` is needed because this test is intended to test that functionality/flag, including the fix in the bitcode reader for the types of functions.
> > 
> > But it might be that this could be separated - the flag could be added and tested purely with IR reading/writing, showing a pointer type going from typed to opaque through the IR roundtrip?
> > 
> > Then separately a commit that shows the bitcode change/fix?
> > 
> > (not sure that explains all of @dexonsmith's questions)
> I read @dexonsmith's question as "are we actually eventually going to update all llvm tests off of `type *` into `ptr`, which my answer is "yes".
> 
> Regarding can we separate the --force-opaque-pointers change and the bitcode change, I believe the answer is no. We need some global or function to show the change, and all of those will require some bitcode change because the type of the global/function is now an opaque pointer, so we have to look at the value type of the global/function.
Sorry for any confusion here.

Taking a fresh look, I see how this tests `--force-opaque-pointers` when reading textual IR, but there's no coverage for reading bitcode. WDYT of this?
```
; RUN: llvm-as --force-opaque-pointers < %s | llvm-dis | FileCheck %s
; RUN: llvm-as < %s | llvm-dis --force-opaque-pointers | FileCheck %s
```

================
Comment at: llvm/tools/llvm-as/llvm-as.cpp:120
   cl::ParseCommandLineOptions(argc, argv, "llvm .ll -> .bc assembler\n");
+  LLVMContext Context;

----------------
Probably better to leave these moves of `LLVMContext` out of this patch -- could be committed separately as an NFC prep-commit ("Moving LLVMContext so that cl::opt can control defaults") if you end up reading the cl::opt during construction.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503