Page MenuHomePhabricator

[OpaquePtr] Forbid mixing typed and opaque pointers
ClosedPublic

Authored by nikic on Sep 5 2021, 8:02 AM.

Details

Reviewers
aeubanks
Group Reviewers
Restricted Project
Commits
rG90ec6dff860f: [OpaquePtr] Forbid mixing typed and opaque pointers
Summary

Currently, opaque pointers are supported in two forms: The -force-opaque-pointers mode, where all pointers are opaque and typed pointers do not exist. And as a simple ptr type that can coexist with typed pointers.

I would like to propose that we remove support for the mixed mode. You either get typed pointers, or you get opaque pointers, but not both. In the (current) default mode, using ptr is forbidden. In -opaque-pointers mode, all pointers are opaque.

The motivation here is that the mixed mode introduces additional issues that don't exist in fully opaque mode. D105155 is an example of a design problem. Looking at D109259, it would probably need additional work to support mixed mode (e.g. to generate GEPs for typed base but opaque result). Mixed mode will also end up inserting many casts between i8* and ptr, which would require significant additional work to consistently avoid.

I don't think the mixed mode is particularly valuable, as it doesn't align with our end goal. The only thing I've found it to be moderately useful for is adding some opaque pointer tests in between typed pointer tests, but I think we can live without that.

Diff Detail

Event Timeline

nikic created this revision.Sep 5 2021, 8:02 AM
nikic requested review of this revision.Sep 5 2021, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2021, 8:02 AM
nikic edited the summary of this revision. (Show Details)Sep 5 2021, 11:26 AM
nikic updated this revision to Diff 371154.Sep 7 2021, 12:46 PM
nikic edited the summary of this revision. (Show Details)

Rebase, fix more codegen tests.

nikic updated this revision to Diff 371158.Sep 7 2021, 12:54 PM
nikic edited the summary of this revision. (Show Details)

Add LLVMContext::enableOpaquePointers() to allow enabling opaque pointers in unit tests without having to touch CLI params.

I think this makes sense, but this may affect how we'll mass-update tests so we should figure out how we're going to transition tests to opaque pointers.

We could make copies of most test but with opaque pointers. I don't like this because if people have to update tests, they'll be updating twice as many. And keeping the copies in sync with the originals will be all but impossible.
We could slowly start adding --opaque-pointers to RUN lines and update tests. Then only when we have different code paths for opaque vs typed pointers do we have duplicate tests. My idea is that if things work with opaque pointers, they should probably also work with typed pointers. @dblaikie wasn't a fan of this due to us potentially losing test coverage for typed pointers.
We could have a bot running all tests with --opaque-pointers and continuously gather crashes. We won't be able to use the number of test failures since CHECK lines will still be wrong. Then mass-migrate all tests in the same change where we flip --opaque-pointers. Personally I don't like a humongous change, even if it's automatable. We'd also have to update a bunch of non-llvm tests in the same change, like clang tests.

Thoughts?

I think this makes sense, but this may affect how we'll mass-update tests so we should figure out how we're going to transition tests to opaque pointers.

We could make copies of most test but with opaque pointers. I don't like this because if people have to update tests, they'll be updating twice as many. And keeping the copies in sync with the originals will be all but impossible.
We could slowly start adding --opaque-pointers to RUN lines and update tests. Then only when we have different code paths for opaque vs typed pointers do we have duplicate tests. My idea is that if things work with opaque pointers, they should probably also work with typed pointers. @dblaikie wasn't a fan of this due to us potentially losing test coverage for typed pointers.
We could have a bot running all tests with --opaque-pointers and continuously gather crashes. We won't be able to use the number of test failures since CHECK lines will still be wrong. Then mass-migrate all tests in the same change where we flip --opaque-pointers. Personally I don't like a humongous change, even if it's automatable. We'd also have to update a bunch of non-llvm tests in the same change, like clang tests.

Thoughts?

My preference was, hopefully, to do one major test transition before the opaque pointer flag-flip to make the tests opaque-agnostic (using regex matches for pointer types so they could match typed or untyped pointers - wouldn't even be averse to makiyng some kind of pre-canned regex in FileCheck for this purpose given the size of the effort involved) - under the theory that we are going to have to do a big test cleanup one way or another, and this way we could avoid losing production test coverage pre-flip, and avoid the flip requiring a huge amount of change. Depends how much more expensive it is to transition tests to be typed-agnostic, compared to just transitioning them from typed to opaque explicitly. If it's not a /lot/ more work, I think it's worth it to keep the production test coverage in the interim.

But if folks (llvm-dev thread may be needed) are generally OK with losing production test coverage in the interim, just migrating to opaque pointer-based testing might be the way to go despite my aversion.

nikic added a comment.Sep 8 2021, 1:12 AM

For most codegen tests (that produce assembly), output for opaque pointers is the same, so that's an easy case :) For IR tests that contain pointers but don't do anything opaque pointer specific, an idea I had was to use these two run lines:

; RUN: opt -S -passes=xxx -opaque-pointers < %s | FileCheck %s
; RUN: opt -S -passes=xxx < %s | opt -S -opaque-pointers | FileCheck %s

What the second line does is run the test with typed pointers and then strip pointer types afterwards, which should usually give it the same output as working with opaque pointers from the start. This way we won't be testing the exact output under typed pointers anymore, but we do test that all pointer types are consistent (due to verifier after first opt invocation), and I think that's all that really matters.

This should allow us to gradually enable opaque pointer testing while ensuring that we're not missing any typed pointer bitcasts.

For most codegen tests (that produce assembly), output for opaque pointers is the same, so that's an easy case :) For IR tests that contain pointers but don't do anything opaque pointer specific, an idea I had was to use these two run lines:

; RUN: opt -S -passes=xxx -opaque-pointers < %s | FileCheck %s
; RUN: opt -S -passes=xxx < %s | opt -S -opaque-pointers | FileCheck %s

What the second line does is run the test with typed pointers and then strip pointer types afterwards, which should usually give it the same output as working with opaque pointers from the start. This way we won't be testing the exact output under typed pointers anymore, but we do test that all pointer types are consistent (due to verifier after first opt invocation), and I think that's all that really matters.

This should allow us to gradually enable opaque pointer testing while ensuring that we're not missing any typed pointer bitcasts.

triples the number of tool invocations, though - which seems a bit concerning. Maybe if we built the option to do the -S -opaque-pointers dance into opt that'd be better for test execution performance?

& having both opaque and non-opaque testing on for everyone (2 x commands for every test) might be a bit much - I was thinking more likely a buildbot/mode where the tests could be run in opaque mode instead, maybe with an "known good" list/marker/indicator of some kind (I guess maybe whatever opt "opacify typed pointers on output" flag could also be seen as an indicator that the test was also acceptable to run in true opaque pointers mod).

aeubanks accepted this revision.Sep 9 2021, 4:09 PM

For most codegen tests (that produce assembly), output for opaque pointers is the same, so that's an easy case :) For IR tests that contain pointers but don't do anything opaque pointer specific, an idea I had was to use these two run lines:

; RUN: opt -S -passes=xxx -opaque-pointers < %s | FileCheck %s
; RUN: opt -S -passes=xxx < %s | opt -S -opaque-pointers | FileCheck %s

What the second line does is run the test with typed pointers and then strip pointer types afterwards, which should usually give it the same output as working with opaque pointers from the start. This way we won't be testing the exact output under typed pointers anymore, but we do test that all pointer types are consistent (due to verifier after first opt invocation), and I think that's all that really matters.

This should allow us to gradually enable opaque pointer testing while ensuring that we're not missing any typed pointer bitcasts.

triples the number of tool invocations, though - which seems a bit concerning. Maybe if we built the option to do the -S -opaque-pointers dance into opt that'd be better for test execution performance?

& having both opaque and non-opaque testing on for everyone (2 x commands for every test) might be a bit much - I was thinking more likely a buildbot/mode where the tests could be run in opaque mode instead, maybe with an "known good" list/marker/indicator of some kind (I guess maybe whatever opt "opacify typed pointers on output" flag could also be seen as an indicator that the test was also acceptable to run in true opaque pointers mod).

We could even have a flag that runs the pipeline twice on the module, once with opaque pointers, another time with typed pointers and a opaquify pass at the end. But with this approach are we going to have the same IR both ways? e.g. an opaque pointer run won't have no-op bitcasts, but an opaque pointer version of the output of a typed pointer run will still have no-op bitcasts from ptr to ptr. We could remove ptr to ptr bitcasts in the opaquify pass, but not sure if there are other differences like this. I think a good amount of tests would have differing IR for the two types of opaque pointer pipeline runs.

I don't think should start mass-updating tests until we're fairly sure that opaque pointers work everywhere. Otherwise people unfamiliar with opaque pointers will have to start seeing them in tests when the opaque pointer transition could be far from done. And at that point we'd definitely have to make a post to llvm-dev to keep people up to speed.

For a buildbot/mode where we worry about opaque pointers, doesn't -(force-)opaque-pointers do that job? Then we run check-llvm and scan for crashes (we can't find incorrect IR like this). We could have a buildbot that runs this and reports the number of tests with crashes, but I think running that locally every couple days is probably the same.

Anyway I think this change is good.

This revision is now accepted and ready to land.Sep 9 2021, 4:09 PM
This revision was landed with ongoing or failed builds.Sep 10 2021, 6:20 AM
This revision was automatically updated to reflect the committed changes.