[4.2-04-30-2018][pred-memopts] Rather than asserting on recursive initialization, jus… #16545
Conversation
…t return false and bail. Until the beginning of the ownership transition, DI and predictable mem opts used the same memory use collector. I split them partially since I need to turn on ownership for predictable mem opts at one time, but also b/c there was a huge amount of special code that would only trigger if it was used by DI or used by predictable mem opts. After I did the copy some of the asserts that were needed for DI remained in the predictable mem opts code. When pred-memopts was only run in the mandatory pipeline keeping these assertions were ok, but pred-memopts was recently added to the perf pipeline meaning that it may see code that breaks these DI invariants (and thus hit this assertion). We should remove this limitation on predictable-memopts but that would require some scheduled time to read the code (more than I have to fix this bug = p). So instead I changed the code to just bail in these cases. rdar://40032102 (cherry picked from commit 269f8e8)
|
@swift-ci test |
gottesmm
changed the title
| %4 = load %0 : $*SWithOpt | ||
| dealloc_stack %0 : $*SWithOpt | ||
| return %4 : $SWithOpt | ||
| } |
|
@shajrawi Joe, since this that doesn't affect anything, I am going to first go through CCC without the newline and then after it gets through CCC add the newline and do a smoke test. I want to make sure that CCC approval sees that this passes the full tests. |
|
Explanation: Eliminates a DI specific assert that pred-memopts could hit. This assert was a vestigal remnant in pred-memopts from when DI and pred-memopts shared common utility code. We did not hit it before since we only used to run pred-memopts right after DI (implying the invariants would still be true). We are now running it in the perf pipeline as well where these invariants are no longer true. Rather than assert or have pred-memopts handle these new untested cases, we are instead conservative in this PR and just bail. |
|
@swift-ci please nominate |
As requested by @shajrawi in swiftlang#16545.
|
Approved by Ted via email. I am think it makes more sense to just merge this now and just add the newline on master. |
…t return false and bail.
Until the beginning of the ownership transition, DI and predictable mem opts
used the same memory use collector. I split them partially since I need to turn
on ownership for predictable mem opts at one time, but also b/c there was a huge
amount of special code that would only trigger if it was used by DI or used by
predictable mem opts. After I did the copy some of the asserts that were needed
for DI remained in the predictable mem opts code. When pred-memopts was only run
in the mandatory pipeline keeping these assertions were ok, but pred-memopts was
recently added to the perf pipeline meaning that it may see code that breaks
these DI invariants (and thus hit this assertion).
We should remove this limitation on predictable-memopts but that would require
some scheduled time to read the code (more than I have to fix this bug = p). So
instead I changed the code to just bail in these cases.
rdar://40032102
(cherry picked from commit 269f8e8)