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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Bitcode/Writer/ValueEnumerator.cpp:366-389
   // Enumerate the global variables.
-  for (const GlobalVariable &GV : M.globals())
+  for (const GlobalVariable &GV : M.globals()) {
     EnumerateValue(&GV);
+    EnumerateType(GV.getValueType());
+  }
 
   // Enumerate the functions.
----------------
dblaikie wrote:
> Looks like maybe only one of these code changes is tested - perhaps the test should be expanded to have an example of each of these to exercise each fix here? (maybe in separate patches)
sure, I can start with just functions


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


================
Comment at: llvm/test/Other/force-opaque-ptrs.ll:1
+; RUN: llvm-as --force-opaque-pointers < %s | llvm-dis | FileCheck %s
+
----------------
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.


================
Comment at: llvm/test/Other/force-opaque-ptrs.ll:5-7
+define i32* @f(i32* %a) {
+  ret i32* %a
+}
----------------
dblaikie wrote:
> Would this still exercise the desired codepath if the function was `void()` instead of `ptr(ptr)`? (if I'm understanding correctly, the interesting issue is that the type of `f` itself becomes an opaque pointer - so, oh, you need /some/ interesting type in the function type that would be missed, yeah? What about `void(i32)` would that still reveal the issue/validate the fix?)
> 
> I ask because it might be more clear which pointer type becoming opaque is the interesting one?
this patch is definitely mixing two things, forcing all pointers to be opaque, and the ValueEnumerator stuff

I'm trying to add a test to make sure that we get out a `ptr` from something that was originally `i32*`.
I can make `@f` only have one visible pointer type:
```
define void @f(i32* %a) {
  ret void
}
```
would turn into
```
define void @f(ptr %a) {
  ret void
}
```
validating this change.

But just testing that doesn't work due to the ValueEnumerator issues introduced by the flag, which is only reproducible with the flag turned on. So if we want to properly test this we need to combine the two patches.

As for what function types cause the crash, even `void()` causes a crash. But that doesn't show that the flag is working as expected since there's no `i32*` anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503