SILCombine: fix a miscompile caused by dead-apply elimination #21803
Conversation
|
@swift-ci test |
|
@swift-ci benchmark |
| // TODO: this is not required anymore when we have ownership SIL. But with | ||
| // the current SIL it can happen that the retain of a parameter is moved | ||
| // _after_ the apply. | ||
| // When we have ownerhip SIL we can just destroy the parameters at the apply |
| @@ -556,6 +563,12 @@ bool SILCombiner::eraseApply(FullApplySite FAS, const UserListTy &Users) { | |||
| } else { | |||
| if (!VLA.computeFrontier(Frontier, ValueLifetimeAnalysis::DontModifyCFG)) | |||
| return false; | |||
| // As we are extending the lifetimes of owned parameters, we have to make | |||
| // sure that no dealloc_ref instructions are within this extended liferange. | |||
| // It could be that the dealloc_ref is deallocting a parameter and then | |||
There was a problem hiding this comment.
deallocting => deallocating
| @@ -282,6 +282,9 @@ class ValueLifetimeAnalysis { | |||
| return LiveBlocks.count(BB) && BB != DefValue->getParent(); | |||
| } | |||
|
|
|||
| // Checks if there is a dealloc_ref inside the value's liferange. | |||
There was a problem hiding this comment.
I think we have been using the term "live range" instead of life range, no? Also this is an interface so it should be a doxygen comment (i.e. 3 slashes).
| @@ -1369,6 +1369,37 @@ bool ValueLifetimeAnalysis::isWithinLifetime(SILInstruction *Inst) { | |||
| llvm_unreachable("Expected to find use of value in block!"); | |||
| } | |||
|
|
|||
| static bool blockContainsDeallocRef(SILBasicBlock *BB, SILInstruction *DefValue, | |||
There was a problem hiding this comment.
Can you add a comment here describing what you are doing? I would say something like:
Searches \p BB backwards from \p FrontierInst to the beginning of the list and returns true if we find a dealloc_ref /before/ we find the instruction that defines our target value.
Also, can you rename DefValue => ValueDefiningInst? Then it is really clear.
There was a problem hiding this comment.
I'll keep DefValue, because it's consistent with ValueLifetimeAnalsis::DefValue
| return false; | ||
| } | ||
|
|
||
| bool ValueLifetimeAnalysis::containsDeallocRef(const Frontier &Frontier) { |
There was a problem hiding this comment.
I think this could use a comment as well. I know what you are doing here, but I had to read the code. I should be able to understand without reading the body basically what this is doing.
|
I'm not sure why we are moving the retain. But with Ownership SIL we will not have the problem anymore, anyway |
Build comment file:Performance: -Onone
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 |
SILCombine ended up moving a strong_release past a dealloc_ref. fixes https://bugs.swift.org/browse/SR-9627 rdar://problem/47153896
eeckstein
force-pushed
the
fix-erase-apply
branch
from
26ec1a7 to
1072bb6
Compare
|
@swift-ci smoke test |
SILCombine ended up moving a strong_release past a dealloc_ref.
fixes https://bugs.swift.org/browse/SR-9627
rdar://problem/47153896