Page MenuHomePhabricator

[OpaquePtr] Introduce option to force all pointers to be opaque pointers
Needs ReviewPublic

Authored by aeubanks on Tue, Jun 1, 9:38 PM.

Details

Reviewers
dblaikie
Summary

We don't want to start updating tests to use opaque pointers until we're
close to the opaque pointer transition. However, before the transition
we want to run tests as if pointers are opaque pointers to see if there
are any crashes.

At some point when we have a flag to only create opaque pointers in the
bitcode and textual IR readers, and when we have fixed all places that
try to read a pointee type, this flag will be useless. However, until
then, this can help us find issues more easily.

Previously ValueEnumerator would visit the value types of global values
via the pointer type, but with opaque pointers we have to manually visit
the value type.

Diff Detail

Event Timeline

aeubanks created this revision.Tue, Jun 1, 9:38 PM
aeubanks requested review of this revision.Tue, Jun 1, 9:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 1, 9:38 PM
dblaikie added inline comments.Wed, Jun 2, 9:25 AM
llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
366–384

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)

llvm/test/Other/force-opaque-ptrs.ll
6–8

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?

dexonsmith added inline comments.Wed, Jun 2, 11:55 AM
llvm/lib/IR/Type.cpp
688–689

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...)

llvm/test/Other/force-opaque-ptrs.ll
2

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?

aeubanks updated this revision to Diff 349594.Thu, Jun 3, 9:53 AM

only touch functions in ValueEnumerator
move ForceOpaquePointers to LLVMContextImpl (this doesn't currently work in some tools since LLVMContext is created before parsing command line opts)

aeubanks edited the summary of this revision. (Show Details)Thu, Jun 3, 9:54 AM
aeubanks added inline comments.Thu, Jun 3, 9:56 AM
llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
366–384

sure, I can start with just functions

llvm/lib/IR/Type.cpp
688–689

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.

llvm/test/Other/force-opaque-ptrs.ll
2

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.

6–8

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.

dblaikie added inline comments.Thu, Jun 3, 7:49 PM
llvm/test/Other/force-opaque-ptrs.ll
2

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)