-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[LVA] Update ignore instructions RLE and DSE. #31134
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
d1dc61c
to
8d946d8
Compare
// If the strong release operand type can't have a custom destructor it's | ||
// OK. | ||
if (release->getOperand()->getType().getClassOrBoundGenericClass() == | ||
nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if the operand itself is not a class, it could be something (e.g. a thick function pointer) which contains a class (e.g. in the closure context).
So it's not safe to assume if it's not a class it cannot (indirectly) call a custom destructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I'll remove this check (ideally, I'd like to use something like dead object elimination's doesDestructorHaveSideEffects
).
// We will look at all of its users and we can project to the source memory | ||
// location so, if there are any actual writes to memory we will catch them | ||
// there so, we can ignore begin_access here. | ||
if (isa<BeginAccessInst>(Inst)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this to isRLEInertInstruction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're, right. That's probably a better place for it.
return; | ||
|
||
// If the load is valid, then this strong release won't invoke the destructor. | ||
// So we can ignore it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't give much context, sorry. I'll try to explain my reasoning a bit more clearly. The only case where we'd care about the strong_release is when it turns into a destructor with a read/write in it (in this case, only a write, actually). If this load is valid (this load being whatever load we happen to be promoting at any given time), then the strong_release can't have destroyed the objected we're loading before we load it. Does that make sense?
8d946d8
to
06932d0
Compare
I removed the access marker part of this patch. I'll add that back after the AccessPath patch lands. I'd really like to get this committed and it shouldn't be hard to review given the size and (new) scope. |
@swift-ci please test and benchmark |
@swift-ci test |
@swift-ci benchmark |
@swift-ci please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow 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
|
Build failed |
I just ran the benchmarks on my machine and I got very different numbers:
The regressions especially don't make sense to me. I looked at the SIL and IR for the |
06932d0
to
b1d14cb
Compare
I removed the strong_release change from the DSE part of this patch. The failing MacOS test exposed an edge case where |
@swift-ci please benchmark |
@swift-ci please test |
@swift-ci test |
@swift-ci benchmark |
1 similar comment
@swift-ci benchmark |
@swift-ci please test windows platform |
@swift-ci please test os x platform |
@swift-ci please benchmark |
@swift-ci please test OS X platform |
Ping. @atrick @eeckstein this patch should (now) only take a few minutes to review. And I'd really like to get it committed. |
// being whatever load we happen to be promoting at any given time), then the | ||
// strong_release can't have destroyed the objected we're loading before we | ||
// load it. So we can safely ignore the strong_release. | ||
case SILInstructionKind::StrongReleaseInst: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that including strong_release is safe here. A deinit can have any side effects, e.g. writing to global variables, etc. What about (in pseudo SIL)
load %address_of_global_variable
strong_release %unrelated_object
load %address_of_global_variable
and the deinit of the unrelated object contains
store %whatever to %address_of_global_variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you're right. I didn't think of that. I'll remove the strong_release from this patch and make another one that does more deinit analysis.
RLE can ignore end_access, set_deallocating, and dealloc_ref instructions. DSE can ignore end_access and set_deallocating.
b1d14cb
to
34ad7d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@eeckstein fantastic. Thank you for reviewing :) |
@swift-ci please benchmark |
@swift-ci please test |
@swift-ci test and merge |
Build failed |
@swift-ci benchmark |
Note that the compiler already does some analysis of release instructions. See MemoryBehaviorVisitor::visitStrongReleaseInst (which is used by AliasAnalysis, which is used by RLE and DSE). |
@eeckstein good to know. I'll take a look and play around with that. |
Build failed |
Looks like an issue with the bot:
Looking at the history, it seems like the last few pull request tests failed for the same reason (on that bot). |
@swift-ci test OS X platform |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -OsizePerformance: -OnoneCode size: -swiftlibsHow 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
|
Looks like most of the performance was in the strong_release part of this patch 😕It should still unlock some performance in #31423, though. |
@swift-ci test linux platform |
1 similar comment
@swift-ci test linux platform |
Build failed |
That again looks like an unrelated issue. But, that's the third time in a row I've gotten some kind of flakiness from the linux tests. Is there some underlying issue or is it just bad luck? |
@swift-ci please test linux platform |
Looks like it was just bad luck :P |
@@ -160,6 +160,9 @@ static bool isRLEInertInstruction(SILInstruction *Inst) { | |||
case SILInstructionKind::IsEscapingClosureInst: | |||
case SILInstructionKind::IsUniqueInst: | |||
case SILInstructionKind::FixLifetimeInst: | |||
case SILInstructionKind::EndAccessInst: | |||
case SILInstructionKind::SetDeallocatingInst: | |||
case SILInstructionKind::DeallocRefInst: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can centralize this notion in a helper. Just a random thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot that could be centralized between DSE and RLE. They both end up doing the work of the other pass anyway so, it might make sense to combine them.
RLE can ignore end_access, set_deallocating, dealloc_ref, begin_access, and strong_release instructions.
DSE can ignore end_access, set_deallocating, begin_access, and sometimes strong_release.
Requires #31132