[PATCH] D109290: [OpaquePtr] Forbid mixing typed and opaque pointers

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks accepted this revision.
aeubanks added a comment.
This revision is now accepted and ready to land.

In D109290#2990260 <https://reviews.llvm.org/D109290#2990260>, @dblaikie wrote:

> In D109290#2988806 <https://reviews.llvm.org/D109290#2988806>, @nikic wrote:
>
>> 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.


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

https://reviews.llvm.org/D109290