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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103503