[ownership] Move ownership lowering after serialization on stdlibCore. #35872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci test |
|
@swift-ci benchmark |
|
@swift-ci test source compatibility |
|
Build failed |
|
Funny story, the test failure is actually a test that had the regex setup to only pattern match a register up to %9 (that is it wouldn't pattern match %10). I fixed the test so we can do the pattern match appropriately now. |
|
I am going to wait for the benchmarks to run then I am going to do another push with the fix/redo the test |
|
Build failed |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
gottesmm
force-pushed
the
ossa-serialize-then-lower-ownership
branch
from
5debe43 to
b2c5f85
Compare
|
@swift-ci test |
|
@swift-ci benchmark |
|
@swift-ci test source compatibility |
|
@swift-ci build toolchain |
|
Any idea why there is so much churn in the benchmarks? |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
I started to look at Breadcrumbs. I think @_specialize may be broken. |
|
Debug source compat failed due to Jenkins error |
|
preset=buildbot,tools=RA,stdlib=RD,test=non_executable |
|
Testing non-executable macOS as well |
|
Build failed |
|
@swift-ci test linux platform |
|
@swift-ci test source compatibility |
|
@swift-ci asan test |
|
Build failed |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
Build failed |
|
@swift-ci test |
|
Build failed |
|
windows test failure is being fixed by compnerd on another PR. Not from this PR. |
|
Woot! Linux succeeded! Not surprised that it would though (I am not enabling this on Linux in this PR). |
|
Build failed |
|
Funny story, I actually need to check in: swiftlang/llvm-project#2483 |
|
EDIT MG: This is the job that succeeded with the full test. @swift-ci test macOS platform |
|
@swift-ci test windows platform |
|
... Erik merged something and created a merge conflict ... And the merge conflict was just an include line... I am going to smoke test this in since I did a full test. |
… ossa/non-ossa SIL. In SILCombine, we do not want to add or delete edges. We are ok with swapping edges or replacing edges when the CFG structure is preserved. This becomes an issue since by performing this optimization, we are going to get rid of the error parameter but leave a try_apply, breaking SIL invariants. So to do perform this optimization, we would need to convert to an apply and eliminate the error edge, breaking the aforementioned SILCombine invariant. So, just do not perform this for now and leave it to other passes like SimplifyCFG.
…ths where there is dynamically no value by inserting compensating destroys. This commit is fixing two things: 1. In certain cases, we are seeing cases where either SILGen or the optimizer are eliminating destroy_addr along paths where we know that an enum is dynamically trivial. This can not be expressed in OSSA, so I added code to pred-deadalloc-elim so that I check if any of our available values after we finish promoting away an allocation now need to have their consuming use set completed. 2. That led me to discover that in certain cases load [take] that we were promoting were available values of other load [take]. This means that we have a memory safety issue if we promote one load before the other. Consider the following SIL: ``` %mem = alloc_stack store %arg to [init] %mem %0 = load [take] %mem store %0 to [init] %mem %1 = load [take] %mem destroy_value %1 dealloc_stack %mem ``` In this case, if we eliminate %0 before we eliminate %1, we will have a stale pointer to %0. I also took this as an opportunity to turn off predictable mem access opt on SIL that was deserialized canonicalized and non-OSSA SIL. We evidently need to still do this for pred mem opts for perf reasons (not sure why). But I am pretty sure this isn't needed and allows me to avoid some nasty code.
…complete available value when cleaning up takes.
While looking at the performance of the verifier running with -sil-verify-all on the stdlib, I noticed that we are spending ~30% of the total time in the verifier performing this check. Introducing the cache mitigates this issue. I believe the reason is that we were walking for each operand the use list of its associated value which I think is quadratic.
…br that do not involve objects directly. Just to reduce the size of the CFG.
…dress through the phi using a RawPointer. In OSSA, we do not allow for address phis, but in certain cases the logic of LoopRotate really wants them. To work around this issue, I added some code in this PR to loop rotate that as a post-pass fixes up any address phis by inserting address <-> raw pointer adapters and changing the address phi to instead be of raw pointer type.
…that have at least one enum tuple sub-elt. Just until MemoryLifetime can handle enums completely.
…passes. This eliminates some regressions by eliminating phase ordering in between ARCSequenceOpts/inlining with read only functions whose read onlyness is lost after inlining.
LLVM seems to not support this today. With ownership SSA, we now produce these for accelerate for some reason, causing lldb/TestAccelerateSIMD.py to fail. I debugged this with Adrian and we got this fix. I filed the radar below against Adrian to fix. rdar://74287800
There is some sort of ASAN issue that this exposes on Linux, so I am going to do this on Darwin and then debug the Linux issue using ASAN over the weekend/next week.
gottesmm
force-pushed
the
ossa-serialize-then-lower-ownership
branch
from
8119387 to
dd6439d
Compare
|
@swift-ci smoke test and merge |
|
It is identical except in the other one I tweaked the triple conditional to be isMacOS instead of this PR which is isDarwin. |
|
@eeckstein The fix for the (String) Breadcrumbs benchmark regression is pending. rdar://74296355 (OSSA benchmark regression; Breadcrumbs.MutatedUTF16ToIdx 4x slower) We also fixed the NSStringConversion regressions. It think it was Meghana's pipeline change that fixed it. Benchmark data with fixes: I think there are still a few significant regressions at -Osize that don't show up at -O, so that's inlining related. |
|
@gottesmm @meg-gupta 👏 👏 👏 So great to see this landing! |
|
I am trying to find clues about some breakage in the Android ARMv7 CI from yesterday. The test that broke is I tried bisecting the problem (carefully enabling the test) and I end up in the commits of #36025, which enables this for non-Darwin platforms. Then I tested again these changes and the test pass when I tried comparing the previous solution in #35776 with the code introduced here, but I don't clearly see where the same solution might be possible (it might just be a red herring). I was wondering if someone has any idea of what might be going wrong and if it can be fixed. Thanks! |
drodriguez
added a commit
to drodriguez/swift
that referenced
this pull request
Feb 27, 2021
It is failing for unknown reasons, probably as a result of swiftlang#35872. XFAILing for the time being to have a green build again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Title says it all. This has a few small PRs to fix issues that I found. I am probably going to test and then rebase.