Skip to content

[pmo] Remove untested code around load_weak, store_weak, load_unowned… #22011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

gottesmm
Copy link
Contributor

…, store_unowned.

I am removing these for the following reasons:

  • PMO does not have any tests for these code paths. (1).

  • PMO does not try to promote these loads (it explicitly pattern matches load,
    copy_addr) or get available values from these (it explicitly pattern matches
    store or explodes a copy_addr to get the copy_addr's stores). This means that
    removing this code will not effect our constant propagation diagnostics. So,
    removing this untested code path at worst could cause us to no longer
    eliminate some dead objects that we otherwise would be able to eliminate at
    -Onone (low-priority). (2).


(1). I believe that the lack of PMO tests is due to this being a vestigal
remnant of DI code in PMO. My suspicion arises since:

 * The code was added when the two passes were both sharing the same use
   collector and auxillary data structures. Since then I have changed DI/PMO
   to each have their own copies.

 * DI has a bunch of tests that verify behavior around these instructions.

(2). I expect the number of actually removed allocations that are no longer
removed should be small since we do not promote loads from such allocations
and PMO will not eliminate an allocation that has any loads.

…, store_unowned.

I am removing these for the following reasons:

* PMO does not have any tests for these code paths. (1).

* PMO does not try to promote these loads (it explicitly pattern matches load,
  copy_addr) or get available values from these (it explicitly pattern matches
  store or explodes a copy_addr to get the copy_addr's stores). This means that
  removing this code will not effect our constant propagation diagnostics. So,
  removing this untested code path at worst could cause us to no longer
  eliminate some dead objects that we otherwise would be able to eliminate at
  -Onone (low-priority). (2).

----

(1). I believe that the lack of PMO tests is due to this being a vestigal
     remnant of DI code in PMO. My suspicion arises since:

     * The code was added when the two passes were both sharing the same use
       collector and auxillary data structures. Since then I have changed DI/PMO
       to each have their own copies.

     * DI has a bunch of tests that verify behavior around these instructions.

(2). I expect the number of actually removed allocations that are no longer
     removed should be small since we do not promote loads from such allocations
     and PMO will not eliminate an allocation that has any loads.
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit babee8b into swiftlang:master Jan 20, 2019
@gottesmm gottesmm deleted the pr-5ea10976b025e6c8196e231982b4395362fc62c4 branch January 20, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants