Simplify AccessEnforcementOpts by leaving access markers in place. #16835
Conversation
The extra cost of deterministically deleting instructions is unnecessary. In the long term, we'll want to verify that access markers exist after all SIL passes. So just change their enforcement level rather than removing them.
|
@swift-ci test. |
|
@shajrawi, could you review the reworded comments? |
| /// access. (Some Unidentified accesses are for initialization or for | ||
| /// temporary storage instead, but those should never have Dynamic | ||
| /// enforcement). These accesses can only be eliminated when there is no | ||
| /// Unidentified access within the function without the [no_nested_conflict] |
There was a problem hiding this comment.
The wording in the last sentence here is a little bit confusing. had to re-read it a couple of times to understand. could be easier to understand if an example is show. or if it is expanded to explain how the presence of a [no_nested_conflict] - less access prevents us from eliminating accesses.
Otherwise LGTM!
There was a problem hiding this comment.
Yeah, thanks. I changed it to simply read:
/// - Stack or Box local storage could potentially be accessed via Unidentified
/// access. (Some Unidentified accesses are for initialization or for
/// temporary storage instead, but those should always have Unsafe
/// enforcement). If this function contains an Unidentified access without its
/// [no_nested_conflict] flag set, then any of the function's local storage
/// could have a nested conflict.
There are comments elsewhere in the code related to it.
This just simplifies the code a bit and does less work. The effect should be the same since this is meant to be run very late in the pipeline and we want to teach all SIL passes to gracefully handle access markers.