swift
[SILOptimizer] Fixes the ValueDefUseWalker to handle potential unchecked_ref_cast between optional and non-optional class types.
#79872
Merged

[SILOptimizer] Fixes the ValueDefUseWalker to handle potential unchecked_ref_cast between optional and non-optional class types. #79872

WeZZard
WeZZard140 days ago👍 4

The unchecked_ref_cast is designed to be able to cast between Optional<ClassType> and ClassType. We need to handle these cases by checking if the type is optional and adjust the path accordingly.

The absence of this process causes the escape analysis for the operand of the load instruction returns non-escaping if there is a unchecked_ref_cast casting from an optional class type to a non-optional class type. Since this the implicit optional-wrapping does not reflect on the path.

This further makes the redundant load elimination pass to eliminate this load though there is a function with side-effects applied right before the load instruction. Since the function apply effect computation in the alias analysis only returns the effects of the function itself if the operand of the load instruction is not escaped.

WeZZard WeZZard requested a review 140 days ago
WeZZard WeZZard requested a review from hborla hborla 140 days ago
WeZZard WeZZard requested a review from slavapestov slavapestov 140 days ago
WeZZard WeZZard requested a review from xedin xedin 140 days ago
WeZZard WeZZard requested a review from eeckstein eeckstein 140 days ago
WeZZard WeZZard requested a review from rjmccall rjmccall 140 days ago
WeZZard WeZZard requested a review from ahoppen ahoppen 140 days ago
WeZZard WeZZard requested a review from bnbarham bnbarham 140 days ago
WeZZard WeZZard requested a review from rintaro rintaro 140 days ago
WeZZard WeZZard requested a review from hamishknight hamishknight 140 days ago
WeZZard WeZZard requested a review from shahmishal shahmishal 140 days ago
WeZZard WeZZard requested a review from mikeash mikeash 140 days ago
WeZZard WeZZard requested a review from al45tair al45tair 140 days ago
WeZZard WeZZard requested a review from compnerd compnerd 140 days ago
WeZZard WeZZard requested a review from ktoso ktoso 140 days ago
WeZZard WeZZard requested a review from AnthonyLatsis AnthonyLatsis 140 days ago
WeZZard WeZZard requested a review from xymus xymus 140 days ago
WeZZard WeZZard requested a review from artemcm artemcm 140 days ago
WeZZard WeZZard requested a review from kavon kavon 140 days ago
WeZZard WeZZard requested a review from jckarter jckarter 140 days ago
WeZZard WeZZard requested a review from CodaFi CodaFi 140 days ago
WeZZard WeZZard requested a review from DougGregor DougGregor 140 days ago
WeZZard WeZZard requested a review from zoecarver zoecarver 140 days ago
WeZZard WeZZard requested a review from hyp hyp 140 days ago
WeZZard WeZZard requested a review from egorzhdan egorzhdan 140 days ago
WeZZard WeZZard requested a review from Xazax-hun Xazax-hun 140 days ago
WeZZard WeZZard requested a review from j-hui j-hui 140 days ago
WeZZard WeZZard requested a review from fahadnayyar fahadnayyar 140 days ago
WeZZard WeZZard requested a review from tshortli tshortli 140 days ago
WeZZard WeZZard requested a review from adrian-prantl adrian-prantl 140 days ago
WeZZard WeZZard requested a review from cachemeifyoucan cachemeifyoucan 140 days ago
WeZZard WeZZard requested a review from beccadax beccadax 140 days ago
WeZZard WeZZard requested a review from ian-twilightcoder ian-twilightcoder 140 days ago
WeZZard WeZZard requested a review from etcwilde etcwilde 140 days ago
WeZZard WeZZard requested a review from edymtt edymtt 140 days ago
WeZZard WeZZard requested a review from justice-adams-apple justice-adams-apple 140 days ago
WeZZard
WeZZard140 days ago

Relative issue: #79871
My blogpost explained this issue and the fix solution (may be tedious): https://wezzard.com/post/2025/03/when-the-swift-compiler-deleted-code-in-stdlib-9067

WeZZard WeZZard changed the base branch from main to release/6.1 140 days ago
WeZZard WeZZard requested a review 140 days ago
WeZZard WeZZard force pushed from 2c5219f6 to 21757d02 140 days ago
Kyle-Ye
Kyle-Ye139 days ago

@swift-ci please test

WeZZard [SILOptimizer] Fixes the ValueDefUseWalker to handle potential unchec…
35f82ba4
WeZZard WeZZard force pushed from 21757d02 to 35f82ba4 139 days ago
WeZZard WeZZard changed the base branch from release/6.1 to main 139 days ago
WeZZard
WeZZard139 days ago👍 1

Hi, folks. I've just learned that the practice of submitting pull requests in the Swift repository is to merge to the main branch instead of a specific releasing branch. I've updated the targeting branch of the PR and fixed a build issue introduced in the tidying up of the commit history. Could anyone help to trigger the @swift-ci to test my commit?

jamieQ
jamieQ139 days ago

@swift-ci please test

eeckstein
eeckstein requested changes on 2025-03-10
eeckstein139 days ago

Thanks for fixing this!

Conversation is marked as resolved
Show resolved
SwiftCompilerSources/Sources/SIL/Type.swift
6363 public var isFunction: Bool { bridged.isFunction() }
6464 public var isMetatype: Bool { bridged.isMetatype() }
6565 public var isClassExistential: Bool { bridged.isClassExistential() }
66
public var isOptional: Bool { bridged.isOptional() }
eeckstein139 days ago

It would be better to define this as astType.isOptional and add the bridging for AST.Type (in TypeProperties)

WeZZard139 days ago

Thanks for the correction. I would like to modify the implementation of this function this way.

Conversation is marked as resolved
Show resolved
SwiftCompilerSources/Sources/SIL/Utilities/WalkUtils.swift
379 if walkDownUses(ofValue: urc, path: path.push(.enumCase, index: 0)) == .abortWalk {
380 return .abortWalk
381 }
382
return walkDownUses(ofValue: urc, path: path.push(.enumCase, index: 1))
eeckstein139 days ago

Why rerun walkDownUses again if the first try doesn't succeed?
I think you should just return walkDownUses(ofValue: urc, path: path.push(.enumCase, index: 0))
Like it's done for EnumInst.

WeZZard139 days ago

The implementation for the walk-down of URC was initially much like what you described. But later then I realized that there might be something lied between the result of the URC and the ultimate use at the end of this def-use chain. Since ValueUseDefWalker is an abstraction for analyses in several function passes, I was worrying about dropping the none case for the Optional at this point may cause the rest analyses not work at intended.

WeZZard139 days ago

May I ask a follow-up question? If we want to handle the implicit casting between Optional and a non-Optional class type by focusing on the some case, should we use path.push(.enumCase, index: 1) rather than 0? It appears that case 1 corresponds to the some case. This might also apply to the walk-up function, where the path manipulation would instead be popIfMatches(.enumCase, index: 1).

WeZZard139 days ago

Now I fixed the walk-down of the def-use chain for URC as you shown in this comment but with case 1. This modification passes all the newly added tests. But I think it might need further discussion.

eeckstein139 days ago

I was worrying about dropping the none case for the Optional

A cast between optional and non-optional implies that it's the some case.

should we use path.push(.enumCase, index: 1) rather than 0?

Yes, of course! My mistake.

Conversation is marked as resolved
Show resolved
SwiftCompilerSources/Sources/SIL/Utilities/WalkUtils.swift
384 if let path = path.popIfMatches(.enumCase, index: 0) {
385 if walkDownUses(ofValue: urc, path: path) == .abortWalk {
386 return .abortWalk
387
} else if let path = path.popIfMatches(.enumCase, index: 1) {
eeckstein139 days ago

Again, this should be exactly like the UncheckedEnumDataInst

Conversation is marked as resolved
Show resolved
SwiftCompilerSources/Sources/SIL/Utilities/WalkUtils.swift
387 } else if let path = path.popIfMatches(.enumCase, index: 1) {
388 return walkDownUses(ofValue: urc, path: path)
389 }
390
}
eeckstein139 days ago

In the path doesn't match the enum case you should call unmatchedPath (see the UncheckedEnumDataInst case)

Conversation is marked as resolved
Show resolved
SwiftCompilerSources/Sources/SIL/Utilities/WalkUtils.swift
729 if let path = path.popIfMatches(.enumCase, index: 0) {
730 if walkUp(value: urc.fromInstance, path: path) == .abortWalk {
731 return .abortWalk
732
} else if let path = path.popIfMatches(.enumCase, index: 1) {
eeckstein139 days ago

The same here

WeZZard139 days ago

The implementation for the walk-up of URC was initially much like what you described. But later then I realized that there might be something lied between the operand of URC and the ultimate enum $Optional<AnyObject>, #Optional.none!enumelt at the beginning of this use-def chain. Since ValueUseDefWalker is an abstraction for analyses in several function passes, I was worrying about dropping the none case for the Optional at this point may cause the rest analyses not work at intended.

Conversation is marked as resolved
Show resolved
SwiftCompilerSources/Sources/SIL/Utilities/WalkUtils.swift
738 if walkUp(value: urc.fromInstance, path: path.push(.enumCase, index: 0)) == .abortWalk {
739 return .abortWalk
740 } else {
741
return walkUp(value: urc.fromInstance, path: path.push(.enumCase, index: 1))
eeckstein139 days ago

And here

Conversation is marked as resolved
Show resolved
include/swift/AST/Types.h
795795 /// Determine whether the type is an opened existential type with Error inside
796796 bool isOpenedExistentialWithError();
797797
798
/// Determine whether the type is an Optional type.
eeckstein139 days ago❤ 1

TypeBase already has an isOptional. It's defined by including "KnownStdlibTypes.def":

#define KNOWN_STDLIB_TYPE_DECL(NAME, DECL_CLASS, NUM_GENERIC_PARAMS) \

Conversation is marked as resolved
Show resolved
test/SILOptimizer/redundant_load_elim_with_casts.sil
588 release_value %4 : $Optional<E>
589 %13 = tuple ()
590 return %13 : $()
591
}
eeckstein139 days ago

Can you also add two tests in test/SILOptimizer/escape_info.sil? Similar to the existing @test_unchecked_ref_cast test.

One test which tests optional -> non-optional and a second test for the other direction.

WeZZard139 days ago❤ 1

I've now added test cases in test/SILOptimizer/escape_info.sil as you suggested. Could please review them to ensure they’re sound?

WeZZard [SILOptimizer] Adds test cases for the escaping analysis of .
775e28f7
WeZZard [SILOptimizer] Refines the implementation of of the bridged SILType …
2237faca
WeZZard [SILOptimizer] Eliminate non-case handling for implicit Optional non-…
4c479824
eeckstein
eeckstein approved these changes on 2025-03-11
eeckstein139 days ago🚀 1

Thanks, lgtm!

eeckstein
eeckstein139 days ago
WeZZard
WeZZard138 days ago

Hi folks! The tests have completed. I think it's time to complete the code review. Do you have any further advice for this pull request?

Plus, since the reviewers list is incredibly long, I wanna make sure that if this pull request really need to involve so much reviewers? Or we just need to involve those reviewers with the shield icon? Reviewers without the shield icon might be mis-involved because I was originally base with release/6.1 but targeted to the main branch. Now the base and the target branch are both fixed to the main branch and matched.

ahoppen ahoppen removed review request from ahoppen ahoppen 138 days ago
eeckstein
eeckstein138 days ago❤ 1

Or we just need to involve those reviewers with the shield icon?

It's fine if a single reviewer (who understands the code) gives her/his thumbs up.
But it's always a good idea to add a few reviewers in case anyone else wants to take a look.

eeckstein eeckstein merged 19c51e2a into main 138 days ago

Login to write a write a comment.

Login via GitHub