-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix findAccessedStorage to handle cyclic phis. #21665
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
@swift-ci test. |
@swift-ci test source compatibility. |
@shajrawi I'm still writing a unit test, but the code can be reviewed in the meantime. |
@shajrawi I included the unit test. |
@swift-ci smoke test. |
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.
a couple of typos but overall LGTM!
lib/SIL/MemAccessUtils.cpp
Outdated
namespace { | ||
// The result of an accessed storage query. A complete result produces an | ||
// AccessedStorage object, which may or may not be valid. An incomplete result | ||
// produces an SILValue representing the source address for use with deeper |
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.
:s/an SILValue/a SILValue/
lib/SIL/MemAccessUtils.cpp
Outdated
// The result of an accessed storage query. A complete result produces an | ||
// AccessedStorage object, which may or may not be valid. An incomplete result | ||
// produces an SILValue representing the source address for use with deeper | ||
// queries. The source address is not necessarilly a SIL address type because |
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.
:s/necessarilly/necessarily/
return AccessedStorageResult::incomplete(operAddr); | ||
} | ||
return AccessedStorage(); | ||
|
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.
The following huge switch case works / I don't clearly see a better option, but I can see potential future bugs if someone adds new (relevant) SIL instructions / does not update it.
Fixes infinite recursion in findAccessedStorge. This routine was not designed to be recursive, but recursion was recently added as a hack for address-type phis. Naturally, phis can be cyclic, so this was not a correct workaround. This is only a problem with enforce-exclusivity=checked, with which findAccessedStorge is invoked after arbitrary optimization passes that introduce address-type phis. Ultimately, address phis will be disallowed in SIL, but some optimizer passes must be fixed first. Fixes <rdar://problem/47059671> swiftlang-1001.0.31.11 root: swiftc crashes
@swift-ci smoke test. |
@swift-ci smoke test OS X platform. |
Fixes infinite recursion in findAccessedStorge. This routine was not
designed to be recursive, but recursion was recently added as a hack
for address-type phis. Naturally, phis can be cyclic, so this was not
a correct workaround.
This is only a problem with enforce-exclusivity=checked, with which
findAccessedStorge is invoked after arbitrary optimization passes that
introduce address-type phis. Ultimately, address phis will be
disallowed in SIL, but some optimizer passes must be fixed first.
Fixes rdar://problem/47059671 swiftlang-1001.0.31.11 root: swiftc crashes