Rework optimization of global variables #65764
Conversation
|
@swift-ci test |
|
@swift-ci benchmark |
…expression evaluator
* Check if the address in question is even visible from outside the function * Return the memory effects of the called function Also, add a new API `Instruction.memoryEffects`, which is internally used by `mayReadFromMemory` et al.
When compiling with optimizations, unconditionally remove `debug_step`
…structure_tuple` Eliminate the redundant instruction pair ``` %t = tuple (%0, %1, %2) (%3, %4, %5) = destructure_tuple %t ``` and replace the results %3, %4, %5 with %0, %1, %2, respectively. The same for structs.
Replace tuple_extract(tuple(x)) -> x
If the callee is side effect-free we can remove the whole builtin "once".
and let its description return a dump of the whole module. This is useful for debugging
A pass is skipped if no other pass changed the function since the previous run of the same pass. Don't do this is if a pass depends on the function bodies of called functions, e.g. the inliner. Other passes might change the callees, e.g. function signature opts, which makes it worth to run the inliner again, even if the function itself didn't change.
`builtin "once"` calls are a result of inlining global accessors
`builtin "once"` calls are a result of inlining global accessors
In case a global accessor is inlined, the `global_addr` and it's `builtin "once"` call is visible in the SIL instead of the call to the global initializer.
Now that we handle inlined global initializers in LICM, CSE and the StringOptimization, we don't need to have a separate mid-level inliner pass, which treats global accessors specially.
…alker Instead just ignore it.
* `Context.copyStaticInitializer(fromInitValue:, to:)` * `FunctionPassContext.createStaticInitializer(for:,initValue:)`
It converts a lazily initialized global to a statically initialized global variable.
When this pass runs on a global initializer `[global_init_once_fn]` it tries to create a static initializer for the initialized global.
```
sil [global_init_once_fn] @globalinit {
alloc_global @the_global
%a = global_addr @the_global
%i = some_const_initializer_insts
store %i to %a
}
```
The pass creates a static initializer for the global:
```
sil_global @the_global = {
%initval = some_const_initializer_insts
}
```
and removes the allocation and store instructions from the initializer function:
```
sil [global_init_once_fn] @globalinit {
%a = global_addr @the_global
%i = some_const_initializer_insts
}
```
The initializer then becomes a side-effect free function which let's the builtin-simplification remove the `builtin "once"` which calls the initializer.
Replace loads of global let variables with there static initializer values.
It marks global `var` variables as `let` if they are never written.
…cGlobals and ReadOnlyGlobalVariablesPass passes.
|
@swift-ci smoke test |
|
@swift-ci smoke test windows |
There was a problem hiding this comment.
@eeckstein This is really great in terms of simplifying the compiler architecture, improving the optimization design, and eliminating confusing code on the C++ side.
The issues I see on implementation side are relatively minor now, but we need to consider setting precedent in new code base. It isn't obvious yet in this PR, but the use of extensions in the Swift sources to tack on arbitrary local functionality will gradually lead to bloated interfaces, incomprehensible layering, and widespread implicit self which makes the code unreadable. You don't need take my word for it, I'm just initiating the discussion you should have with others.
| @@ -71,6 +71,33 @@ extension MutatingContext { | |||
| erase(instruction: inst) | |||
| } | |||
|
|
|||
| /// Copies all instructions of a static init value of a global to the insertion point of `builder`. | |||
| func copyStaticInitializer(fromInitValue: Value, to builder: Builder) -> Value? { | |||
There was a problem hiding this comment.
I'm a little alarmed that the basic pass context interface includes arbitrary utilities, like copyStaticInitializer and tryDeleteDeadClosure, which are not fundamental to a pass context and do not require access to the context's internal state.
The optimizer should have a separate place where arbitrary function-level transformations can be implemented.
There was a problem hiding this comment.
In general, all SIL modifications must go through context, for two reasons:
- Automatic change notifications for analysis invalidation
- In future, it allows the context to be an "actor" in a concurrent pass pipeline. E.g. we can verify that only the current function is modified and not e.g. they body of a called function, etc.
Optimizer utilities can be either
- a member of the context, directly
- a free standing function, which gets the context and the SIL entity (e.g. the
Function) as parameters - a method in an extension of a SIL entity (e.g.
Function) which gets the context as a parameter
I'm not sure what's the best way. But yes, it makes sense to agree on a "coding style" here.
| /// %i = some_const_initializer_insts | ||
| /// store %i to %a | ||
| /// ``` | ||
| func getGlobalInitialization() -> (allocInst: AllocGlobalInst, storeToGlobal: StoreInst)? { |
There was a problem hiding this comment.
I'm baffled as to why getGlobalInitialization is a member of Function as opposed to taking a Function as an argument.
As a rule, if it doesn't require access to Function's internal state, don't make it part of Function's interface.
The implicit self in the implementation also makes it very hard to read the code.
There was a problem hiding this comment.
As a rule, if it doesn't require access to Function's internal state, don't make it part of Function's interface.
Note that in an extension you can't access internal state and a private extension doesn't add to the type's interface.
| } | ||
|
|
||
| private extension Value { | ||
| var addressHasWrites: Bool { |
There was a problem hiding this comment.
It's very strange to me that you would hide the functionality of def-use traversal inside the unnecessary abstractions of a computed property within an extension of value. If something is initiating a def-use walk, I really want that to be obvious at the point-of-use. It's quite important that people don't accidentally perform repeated def-use walks.
There was a problem hiding this comment.
Well, a computed property is not more of an abstraction than a method.
But I agree that a property conveys that it's something very simple (O(1) complexity) and doesn't involve any (potential) computational expensive operations.
So in this case it's in fact better to define it as a method.
There was a problem hiding this comment.
Well, a computed property is not more of an abstraction than a method.
But I agree that a property conveys that it's something very simple (O(1) complexity) and doesn't involve any (potential) computational expensive operations.
@eeckstein Yes, you said it better. That's the only point I was making. I've seen the same argument made during review of Swift libraries.
| for f in moduleContext.functions { | ||
| for inst in f.instructions { | ||
| if let gAddr = inst as? GlobalAddrInst { | ||
| if gAddr.addressHasWrites { |
There was a problem hiding this comment.
The most important part of this algorithm is that it performs a def-use traversal. Hiding the most important fact that code needs to convey is the definition of unreadable code.
|
@atrick thanks for reviewing!
I agree, we should agree on an "coding style".
Note that "local functionalities" are inside Also note that you can't access internal/private members in an extension. So, putting local functionality in a private extension is really nothing else than putting it into a private freestanding function, just with different syntax. |
|
Alternative: #65821 |
The goal of this PR is to simplify the optimizations of global variables.
The existing complex GlobalOpt optimization is replaced by individual small optimizations, which are implemented in swift:
InitializeStaticGlobals: converts lazily initialized globals to a statically initialized global variablesReadOnlyGlobalVariables: marks globalvarvariables asletif they are never writtenloadinstructions replaces a load of a statically initialized global let with its initializer valuebuiltininstructions constant folds builtins and removes redundantbuiltin "once"callsThose passes are very small and simple and in total much fewer lines of code than the old GlobalOpt pass.
Except the
ReadOnlyGlobalVariables, those passes also run at Onone and are therefore important for performance annotated functions, which access global variables.This PR also simplifies the inliners in the pass pipeline: three is no distinction between the "mid-level" and "late" inliner anymore. The only difference of the "mid-level" inliner compared to the "late" inliner was that it didn't line global accessors. This PR adds support for inlined global accessors in LICM and CSE (they now can handle
builtin "once"calls). Therefore it's not needed to have both kinds of inliners.The PR contains a few other small changes and fixes which are needed to make this change work. For details see the commit messages.