This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtrs] Add -normalize-opaque-pointers option
AbandonedPublic

Authored by nikic on Jan 20 2022, 5:43 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

This adds a -normalize-opaque-pointers option to opt, which is intended to allow running most of our test both in typed and opaque pointer mode.

In opaque pointer mode, this option does nothing. In typed pointer mode, before we print the module, we first import it into a new context with opaque pointers enabled, and then print the result. This happens after the IR has been verified with typed pointers.

The idea here is that running the verifier is sufficient to ensure that the typed pointer IR is consistent (has all the necessary bitcasts and matching types), and we can then print it with opaque pointers, so the output is the same as in proper opaque pointer mode.

This will not hold up if the pass does anything fancy with opaque pointers, but I think it will allow us to cover a significant portion of tests while still fully supporting both modes. I've included sample EarlyCSE test changes here.

Diff Detail

Event Timeline

nikic requested review of this revision.Jan 20 2022, 5:43 AM
nikic created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 5:43 AM

Could this be done by roundtripping through llvm-dis, rather than by having a builtin feature? (even if it can be done that way, not necessarily suggesting it's the better option - adding extra program executions will slow down tests especially on windows, etc - but curious/considering all the options, etc)

nikic added a comment.Jan 20 2022, 1:34 PM

Could this be done by roundtripping through llvm-dis, rather than by having a builtin feature? (even if it can be done that way, not necessarily suggesting it's the better option - adding extra program executions will slow down tests especially on windows, etc - but curious/considering all the options, etc)

This is actually what I originally proposed in https://reviews.llvm.org/D109290#2988806, and you suggested to integrate the option to save the extra program execution :) Another benefit of the option is that we can also drop the ptr to ptr bitcasts.

Could this be done by roundtripping through llvm-dis, rather than by having a builtin feature? (even if it can be done that way, not necessarily suggesting it's the better option - adding extra program executions will slow down tests especially on windows, etc - but curious/considering all the options, etc)

This is actually what I originally proposed in https://reviews.llvm.org/D109290#2988806, and you suggested to integrate the option to save the extra program execution :) Another benefit of the option is that we can also drop the ptr to ptr bitcasts.

Ah, thanks for the context/reminder.

I think from that thread I still tend towards "introduce a build mode where it reads into opaque pointers (essentially auto-upgrade), either have a list of known-failing tests in this mode, or some other way to flag on a per-test basis (maybe start with a "passing" list, and eventually move to a "failing" list when that's smaller)" - but I don't feel especially strongly/open to others perspectives.

nikic updated this revision to Diff 401886.Jan 21 2022, 12:41 AM

Fix PM test failures, port all (passing) EarlyCSE tests.

I think from that thread I still tend towards "introduce a build mode where it reads into opaque pointers (essentially auto-upgrade),

Flipping the opaque-pointers option to true is effectively that -- we should probably make that default controlled by a cmake option.

either have a list of known-failing tests in this mode, or some other way to flag on a per-test basis (maybe start with a "passing" list, and eventually move to a "failing" list when that's smaller)" - but I don't feel especially strongly/open to others perspectives.

Looking at just Transforms tests, we have about 7k tests, of which 3k fail under opaque pointers, of which 0.5k crash. My primary concern here is that if we don't introduce something like this, we'll need to update 3k tests (in Transforms alone) during the opaque pointers switch. And we'll need to separately maintain those opaque-ified tests prior to the switch.

I think the more changes we can make prior to the atomic "flip the flag" commit, the better.

I think from that thread I still tend towards "introduce a build mode where it reads into opaque pointers (essentially auto-upgrade),

Flipping the opaque-pointers option to true is effectively that -- we should probably make that default controlled by a cmake option.

Yeah, if that essentially enables the shipped version of opaque pointers (minus later API cleanup) - yeah, having a CMake option for that sounds awesome. Then people can try turning it on early, have a way to opt-out at least for a little while, let us run buildbots in the new mode before it ships, and continue testing in the old mode after it ships (briefly - to keep things passing in case we need to rollback/some users need a bit longer to migrate).

either have a list of known-failing tests in this mode, or some other way to flag on a per-test basis (maybe start with a "passing" list, and eventually move to a "failing" list when that's smaller)" - but I don't feel especially strongly/open to others perspectives.

Looking at just Transforms tests, we have about 7k tests, of which 3k fail under opaque pointers, of which 0.5k crash. My primary concern here is that if we don't introduce something like this, we'll need to update 3k tests (in Transforms alone) during the opaque pointers switch. And we'll need to separately maintain those opaque-ified tests prior to the switch.

I think the more changes we can make prior to the atomic "flip the flag" commit, the better.

Oh, for sure/agreed on this last point.

If we had the CMake flag described, and a buildbot for that config - we would update the CHECKs in these 3k tests to pass with/without opaque pointers, yeah? Ah, with the -normalize-opaque-pointers flag we wouldn't have to update these files to have agnostic (accounting for both typed and opaque pointers) CHECKs, they'd only have opaque pointers CHECKs - which is easier to do and means these don't eventually need to be cleaned up (or be weird/legacy phrasing) after the opaque pointers change is shipped?

Yeah, mixed feelings - happy to hear how other folks feel about what the tradeoffs are between these choices.

The issue with the opaque pointer migration is that passes sometimes don't handle them correctly, not that we can't print the final IR. This patch still doesn't test that passes properly handle both typed pointers and opaque pointers, unless the idea is that we'd have RUN lines with both -opaque-pointers and -normalize-opaque-pointers which this patch doesn't do for the modified tests.
As for a flag flip day, it would be per-frontend right? We can already incrementally update tests (including frontend e.g. clang) by adding -opaque-pointers to RUN lines.

llvm/tools/opt/NewPMDriver.cpp
209

not really true when we remove instructions but it's the last pass so whatever

The issue with the opaque pointer migration is that passes sometimes don't handle them correctly, not that we can't print the final IR. This patch still doesn't test that passes properly handle both typed pointers and opaque pointers, unless the idea is that we'd have RUN lines with both -opaque-pointers and -normalize-opaque-pointers which this patch doesn't do for the modified tests.

The idea is that the tests can be run with both -opaque-pointers=0 and -opaque-pointers=1. They will use -opaque-pointers=0 by default, but they can be run with -opaque-pointers=1 and will pass -- for now, that would just be manual runs by interested parties, but if we can have a significant fraction of tests passing, that may be a buildbot.

Of course, another possibility would be to add both a -normalize-opaque-pointers and an `-opaque-pointers' RUN line, so that everyone tests both modes. That would double the number of RUN lines (for tests using pointers) though, but maybe it's preferred?

As for a flag flip day, it would be per-frontend right? We can already incrementally update tests (including frontend e.g. clang) by adding -opaque-pointers to RUN lines.

Adding -opaque-pointers to tests means we lose typed pointer test coverage though. I don't think that's a good idea while typed pointers are still the default mode.

nikic added a comment.Feb 8 2022, 1:39 PM

Any further thoughts on this? Some options I see:

  1. Add -normalize-opaque-pointers to existing RUN lines. This allows them to pass when opaque pointers are enabled, but doesn't test opaque pointers in a default configuration.
  2. Add both -opaque-pointers=1 and -opaque-pointers=0 -normalize-opaque-pointers to RUN lines. This makes everyone test both typed and opaque pointers in a default configuration.
  3. Add both -opaque-pointers=1 and -opaque-pointers=0 to RUN lines with different FileCheck prefixes.
  4. Add -opaque-pointers to RUN lines and stop testing typed pointers.
  5. ???

I think all of these are principally viable, with different tradeoffs. This patch currently proposed version 1.

Any further thoughts on this? Some options I see:

  1. Add -normalize-opaque-pointers to existing RUN lines. This allows them to pass when opaque pointers are enabled, but doesn't test opaque pointers in a default configuration.
  2. Add both -opaque-pointers=1 and -opaque-pointers=0 -normalize-opaque-pointers to RUN lines. This makes everyone test both typed and opaque pointers in a default configuration.
  3. Add both -opaque-pointers=1 and -opaque-pointers=0 to RUN lines with different FileCheck prefixes.
  4. Add -opaque-pointers to RUN lines and stop testing typed pointers.
  5. ???

I think all of these are principally viable, with different tradeoffs. This patch currently proposed version 1.

My assumption is that most things work with opaque pointers and we'll fix and add tests for the few things that specifically are broken by opaque pointers.
I think 1) is fine, although I still sorta prefer 4) but waiting until we've completely flushed out all opaque pointer issues mostly because I don't really want to burden people not working on opaque pointers with another thing to worry about (what is this ptr thing? why does the output IR look slightly different than the input IR? why do all these tests have -normalize-opaque-pointers?) during the opaque pointer fixup period which could last a while. It's possible that opaque pointer issues we aren't specifically testing could regress, but IMO that's unlikely and also easy to pinpoint and fix. But I could be convinced to do 1).
Perhaps one issue is testing of typed pointers post-opaque pointers, which is where 1) would be useful. Not sure how much we want to commit to having rigorous testing of typed pointers when opaque pointers are the default for in tree projects.
I don't like 2) and 3), they're too much overhead for not enough value.

Any further thoughts on this? Some options I see:

  1. Add -normalize-opaque-pointers to existing RUN lines. This allows them to pass when opaque pointers are enabled, but doesn't test opaque pointers in a default configuration.
  2. Add both -opaque-pointers=1 and -opaque-pointers=0 -normalize-opaque-pointers to RUN lines. This makes everyone test both typed and opaque pointers in a default configuration.
  3. Add both -opaque-pointers=1 and -opaque-pointers=0 to RUN lines with different FileCheck prefixes.
  4. Add -opaque-pointers to RUN lines and stop testing typed pointers.
  5. ???

I think all of these are principally viable, with different tradeoffs. This patch currently proposed version 1.

My assumption is that most things work with opaque pointers and we'll fix and add tests for the few things that specifically are broken by opaque pointers.
I think 1) is fine, although I still sorta prefer 4) but waiting until we've completely flushed out all opaque pointer issues mostly because I don't really want to burden people not working on opaque pointers with another thing to worry about (what is this ptr thing? why does the output IR look slightly different than the input IR? why do all these tests have -normalize-opaque-pointers?) during the opaque pointer fixup period which could last a while. It's possible that opaque pointer issues we aren't specifically testing could regress, but IMO that's unlikely and also easy to pinpoint and fix. But I could be convinced to do 1).
Perhaps one issue is testing of typed pointers post-opaque pointers, which is where 1) would be useful. Not sure how much we want to commit to having rigorous testing of typed pointers when opaque pointers are the default for in tree projects.
I don't like 2) and 3), they're too much overhead for not enough value.

Seems like a 5th option might be a buildbot that builds with opaque pointers on by default & a list of known failures suppressed? But maybe not worth the hassle to setup, not sure.

My assumption is that most things work with opaque pointers and we'll fix and add tests for the few things that specifically are broken by opaque pointers.
I think 1) is fine, although I still sorta prefer 4) but waiting until we've completely flushed out all opaque pointer issues mostly because I don't really want to burden people not working on opaque pointers with another thing to worry about (what is this ptr thing? why does the output IR look slightly different than the input IR? why do all these tests have -normalize-opaque-pointers?) during the opaque pointer fixup period which could last a while. It's possible that opaque pointer issues we aren't specifically testing could regress, but IMO that's unlikely and also easy to pinpoint and fix. But I could be convinced to do 1).
Perhaps one issue is testing of typed pointers post-opaque pointers, which is where 1) would be useful. Not sure how much we want to commit to having rigorous testing of typed pointers when opaque pointers are the default for in tree projects.
I don't like 2) and 3), they're too much overhead for not enough value.

Okay, my original thinking here was that we should have most of the llvm lit tests passing with opaque pointers before they get enabled in clang, but on further consideration that may not be particularly valuable. Basically, cases where opaque pointers are improperly handled will either crash outright (which we can easily check), or won't have existing test coverage anyway.

I guess the chicken and egg problem here gets solved by enabling opaque pointers in clang despite having limited direct test coverage in LLVM, and then converting LLVM tests after that has happened (and dropping typed pointer coverage while doing that). That is probably less risky than the other way around (first migrating the tests and then flipping what clang uses) by dint of typed pointers being harder to get right in terms of inserting all the necessary bitcasts.

In that case what I'm proposing here might actually be more useful on the clang side? It would allow us to convert all the clang tests to use opaque pointer output in advance, rather than doing a huge mass change as part of flipping the flag (which is likely going to be reverted a couple of times).

I do think we've reached the point where it's fine to mass-include ptr in tests and make other people deal with opaque pointers. Opaque pointers basically work for end-to-end optimized compiles of C/C++ now, and we're now working on long-tail issues now.

I plan to start a discourse thread soon to get some wider input on how to handle the opaque pointer switch (and make sure that people with out-of-tree code are aware that a switch will happen soon...)

nikic abandoned this revision.May 4 2022, 6:33 AM

Abandoning this as we decided not to pursue this approach.

Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 6:33 AM