Fix mem2reg error due to multi block mem2reg with store_borrows #65367
Conversation
…e_borrows @guaranteed stored values will never be added to a phi in mem2reg because any uses of store borrowed locations have to be accessed via the store borrow return address and since we ban address phis altogether we will never have input sil which necessitates this. But, the DJ-edges based algorithm for inserting phi blocks in mem2reg can insert unnecessary phis which are removed later. Ensure fixPhiPredBlock handles both owned and guaranteed stored values correctly for this reason.
|
@swift-ci test |
|
Incidentally, do these tests fail without this patch? I tried recent main, release/5.9, and release/5.8. Or is # 65345 needed to see them fail? (I haven't tried with that patch.) |
meg-gupta
changed the title
|
@nate-chandler It gets exposed as an assert with #65345. Without #65345 there is https://github.com/apple/swift/blob/1c3667b52aa7888f7e2cc253b725a7ed055e4ef2/lib/SILOptimizer/Transforms/SILMem2Reg.cpp#L1242 where I haven't yet looked if doing https://github.com/apple/swift/blob/1c3667b52aa7888f7e2cc253b725a7ed055e4ef2/lib/SILOptimizer/Transforms/SILMem2Reg.cpp#L1242 unconditionally can have a different implication without #65345 or on 5.9 |
|
Yeah, https://github.com/apple/swift/blob/1c3667b52aa7888f7e2cc253b725a7ed055e4ef2/lib/SILOptimizer/Transforms/SILMem2Reg.cpp#L1242 needs this fix. Just asking about these test cases--looks like we get the same output with or without the fix (without # 65345). |
|
@nate-chandler Yeah, I added this as a separate PR from #65345, because I thought the explanation deserved a new PR, but I guess it makes it unclear in a different way. |
|
Thanks for the explanation! |
@guaranteed stored values will never be added to a phi in mem2reg because any uses of store borrowed locations have to be accessed via the store borrow return address and since we ban address phis altogether we will never have input sil which necessitates this.
But, the DJ-edges based algorithm for inserting phi blocks in mem2reg can insert unnecessary phis which are removed later.
Ensure fixPhiPredBlock handles both owned and guaranteed stored values correctly for this reason.