This patch replace all in-memory objects of DebugValueAddrInst with
DebugValueInst + op_deref, and duplicates logics that handles
DebugValueAddrInst with the latter. All related check in the tests
have been updated as well.
Note that this patch neither remove the DebugValueAddrInst class nor
remove `debug_value_addr` syntax in the test inputs.
In OSSA RLE for loops, in certain cases SSAUpdater will not create a new
SILPhiArgument to be used as the forwarding value. Based on dominator info
it may return the newly copied available value as the forwarding value.
This newly copied available value in the dominating predecessor
will have destroy values at leaking blocks.
Rename makeNewValueAvailable to makeValueAvailable and handle users so that only
additional required destroy_values are inserted.
We were using compiler-generated source location (i.e. line number
0) on `debug_value` instructions were emitted by salvage debug info.
But it turned out that IRGen will attach DW_AT_artificial on the
associated debug variables, which are hidden in debugger by default.
This patch prevent this issue by using the source location from the
just-deleted instruction for these `debug_value` instructions.
This indirectly triggered another bug where some of the LLVM
!DIExpression-s we generated are actually invalid -- more specifically,
the fragment inside is covering the whole debug variable. The reason it was
not previously caught is because LLVM verifier skips any debug variable
that is marked artificial, and all debug variables that have illegal fragment
are falling under this category. Thus, this patch also fixes this issue
by not generating the DW_OP_LLVM_fragment part if it's illegal.
OSSA rauw cleans up end of scope markers before rauw'ing.
This can lead to inadvertant deleting of end_lifetime, later
resulting in an ownership verifier error indicating a leak.
This PR stops treating end_lifetime scope ending like end_borrow/end_access.
SROA and Mem2Reg now can leverage DIExpression -- op_fragment, more
specifically -- to generate correct debug info for optimized SIL. Some
important highlights:
- The new swift::salvageDebugInfo, similar to llvm::salvageDebugInfo,
tries to restore / transfer debug info from a deleted instruction.
Currently I only implemented this for store instruction whose
destination is an alloc_stack value.
- Since we now have source-variable-specific SIL location inside a
`debug_value` instruction (and its friends), this patch teaches
SILCloner and SILInliner to remap the debug scope there in addition
to debug scope of the instruction.
- DCE now does not remove `debug_value` instruction whose associating
with a function argument SSA value that is not used elsewhere. Since
that SSA value will not disappear so we should keep the debug info.
Debug variables that are marked 'implicit' on its `debug_value`
instruction mean that they were generated by compiler. Optimizers are
free to remove them (if it becomes a dead code, for instance) even in
-Onone. Since they are barely used by users and keeping them might lead
to incorrect IRGen results.
Track in-use iterators and update them both when instructions are
deleted and when they are added.
Safe iteration in the presence of arbitrary changes now looks like
this:
for (SILInstruction *inst : deleter.updatingRange(&bb)) {
modify(inst);
}
Clarify the API. Make it suitable for use everywhere in the
compiler. We should try to standardize on it and allow it to do the
OSSA fixup more often.
Add InstructionDeleter::updatingIterator() factory so we never
normally need to use InstModCallbacks.
Fix bugs in which notifyWillBeDeleted() was being called on invalid
SIL. The bugs are easily exposed just by removing copy_value side
effects, but that will be in the follow-up commit.
Call notifyWillBeDeleted() only when identifying new dead instructions
that the client may not know about. Give the client control over
force-deleting instructions. When doing its own lifetime fixups, the
client may force-delete a set of related instructions. Invoking
callbacks for these force-deleted instructions is wrong.
TODO: partial_apply support is only partial. I disabled the buggy
cases. This should be easy to fix but requires designing some
InstructionDeleter test cases.
I recently have been running into the issue that many of these APIs perform the
deletion operation themselves and notify the caller it is going to delete
instead of allowing the caller to specify how the instruction is deleted. This
causes interesting semantic issues (see the loop in deleteInstruction I
simplified) and breaks composition since many parts of the optimizer use
InstModCallbacks for this purpose.
To fix this, I added a notify will be deleted construct to InstModCallback. In a
similar way to the rest of it, if the notify is not set, we do not call any code
implying that we should have good predictable performance in loops since we will
always skip the function call.
I also changed InstModCallback::deleteInst() to notify before deleting so we
have a default safe behavior. All previous use sites of this API do not care
about being notified and the only new use sites of this API are in
InstructionDeleter that perform special notification behavior (it notifies for
certain sets of instructions it is going to delete before it deletes any of
them). To work around this, I added a bool to deleteInst to control this
behavior and defaulted to notifying. This should ensure that all other use sites
still compose correctly.
Instead, put the archetype->instrution map into SIlModule.
SILOpenedArchetypesTracker tried to maintain and reconstruct the mapping locally, e.g. during a use of SILBuilder.
Having a "global" map in SILModule makes the whole logic _much_ simpler.
I'm wondering why we didn't do this in the first place.
This requires that opened archetypes must be unique in a module - which makes sense. This was the case anyway, except for keypath accessors (which I fixed in the previous commit) and in some sil test files.
Refactor SILGen's ApplyOptions into an OptionSet, add a
DoesNotAwait flag to go with DoesNotThrow, and sink it
all down into SILInstruction.h.
Then, replace the isNonThrowing() flag in ApplyInst and
BeginApplyInst with getApplyOptions(), and plumb it
through to TryApplyInst as well.
Set the flag when SILGen emits a sync call to a reasync
function.
When set, this disables the SIL verifier check against
calling async functions from sync functions.
Finally, this allows us to add end-to-end tests for
rdar://problem/71098795.
Enable most simplify-cfg optimizations as long as the block arguments
have trivial types. Enable most simplify CFG unit tests cases.
This massively reduces the size of the CFG during OSSA passes.
Test cases that aren't supported in OSSA yet have been moved to a
separate test file for disabled OSSA tests,
Full simplify-cfg support is currently blocked on OSSA utilities which
I haven't checked in yet.
If we know that we have a FunctionRefInst (and not another variant of FunctionRefBaseInst), we know that getting the referenced function will not be null (in contrast to FunctionRefBaseInst::getReferencedFunctionOrNull).
NFC
Instead make `findJointPostDominatingSet` a stand-alone function.
There is no need to keep the temporary SmallVector alive across multiple calls of findJointPostDominatingSet for the purpose of re-using malloc'ed memory. The worklist usually contains way less elements than its small size.
Some notes:
1. I moved the identity round-trip case to InstSimplify since that is where
optimizations like that are.
2. I did not update in this commit the code that eliminates convert_function
when it is only destroyed. In a subsequent commit I am going to implement
that in a general way and apply it to all forwarding instructions.
3. I implemented eliminating convert_function with ownership only uses in a
utility so that I can reuse it for other similar optimizations in SILCombine.
Currently all of these places in the code base perform simplifyInstruction and
then a replaceAllSimplifiedUsesAndErase(...). This is a bad pattern since:
1. simplifyInstruction assumes its result will be passed to
replaceAllSimplifiedUsesAndErase. So by leaving these as separate things, we
allow for users to pointlessly make this mistake.
2. I am going to implement in a subsequent commit a utility that lifetime
extends interior pointer bases when replacing an address with an interior
pointer derived address. To do this efficiently, I want to reuse state I
compute during simplifyInstruction during the actual RAUW meaning that if the
two operations are split, that is difficult without extending the API. So by
removing this, I can make the transform and eliminate mistakes at the same
time.
Given an Operand *op, this API executes op->set(newSILValue) except that:
1. If the user of op is an end scope, this API no-opts. This API is only used in
contexts where we are rewriting uses and are not interesting in end scope
instructions since we are moving uses from one scope to another scope.
2. If the user of op is not an end scope, but is a lifetime ending use of
op->get(), we insert a destroy_value|end_borrow as appropriate on op->get()
to ensure op->get()'s lifetime is still ended. We assume that if
op->getUser() is lifetime ending, that our caller has ensured that we can end
newValue's lifetime.
This is a low level API already being used in multiple places besides
InstSimplify (e.x.: Utils/OwnershipOptUtils), so it makes sense to move it into
InstOptUtil.
Specifically before this PR, if a caller did not customize a specific callback
of InstModCallbacks, we would store a static default std::function into
InstModCallbacks. This means that we always would have an indirect jump. That is
unfortunate since this code is often called in loops.
In this PR, I eliminate this problem by:
1. I made all of the actual callback std::function in InstModCallback private
and gave them a "Func" postfix (e.x.: deleteInst -> deleteInstFunc).
2. I created public methods with the old callback names to actually call the
callbacks. This ensured that as long as we are not escaping callbacks from
InstModCallback, this PR would not result in the need for any source changes
since we are changing a call of a std::function field to a call to a method.
3. I changed all of the places that were escaping inst mod's callbacks to take
an InstModCallback. We shouldn't be doing that anyway.
4. I changed the default value of each callback in InstModCallbacks to be a
nullptr and changed the public helper methods to check if a callback is
null. If the callback is not null, it is called, otherwise the getter falls
back to an inline default implementation of the operation.
All together this means that the cost of a plain InstModCallback is reduced and
one pays an indirect function cost price as one customizes it further which is
better scalability.
P.S. as a little extra thing, I added a madeChange field onto the
InstModCallback. Now that we have the helpers calling the callbacks, I can
easily insert instrumentation like this, allowing for users to pass in
InstModCallback and see if anything was RAUWed without needing to specify a
callback.
This makes it easier to understand conceptually why a ValueOwnershipKind with
Any ownership is invalid and also allowed me to explicitly document the lattice
that relates ownership constraints/value ownership kinds.
This makes it clearer that isConsumingUse() is not an owned oriented API and
returns also for instructions that end the lifetime of guaranteed values like
end_borrow.
* Fix another use-after-free in SILCombine
swift::endLifetimeAtFrontier also needs to use
swift::emitDestroyOperation and delete instructions via callbacks that
can correctly remove it from the worklist that SILCombine maintains
* Add test for use-after-free in SILCombine
SILCombine maintains a worklist of instructions and deleting of instructions is valid only via callbacks that remove them from the worklist as well. It calls swift::tryDeleteDeadClosure which in turn calls SILBuilder apis like emitStrongRelease/emitReleaseValue/emitDestroyValue which can delete instructions via SILInstruction::eraseFromParent leaving behind a stale entry in SILCombine's worklist causing a crash.
This PR adds swift::emitDestroyOperation which correctly calls the appropriate InstModCallbacks on added/removed instructions. This comes from swift::releasePartialApplyCapturedArg which was handling creation of destroys with callbacks correctly.
Optimize the unconditional_checked_cast_addr in this pattern:
%box = alloc_existential_box $Error, $ConcreteError
%a = project_existential_box $ConcreteError in %b : $Error
store %value to %a : $*ConcreteError
%err = alloc_stack $Error
store %box to %err : $*Error
%dest = alloc_stack $ConcreteError
unconditional_checked_cast_addr Error in %err : $*Error to ConcreteError in %dest : $*ConcreteError
to:
...
retain_value %value : $ConcreteError
destroy_addr %err : $*Error
store %value to %dest $*ConcreteError
This lets the alloc_existential_box become dead and it can be removed in following optimizations.
The same optimization is also done for conditional_checked_cast_addr.
There is also an implication for debugging:
Each "throw" in the code calls the runtime function swift_willThrow. The function is used by the debugger to set a breakpoint and also add hooks.
This optimization can completely eliminate a "throw", including the runtime call.
So, with optimized code, the user might not see the program to break at a throw, whereas in the source code it is actually throwing.
On the other hand, eliminating the existential box is a significant performance win and we don't guarantee any debugging behavior for optimized code anyway. So I think this is a reasonable trade-off.
I added an option "-Xllvm -keep-will-throw-call" to keep the runtime call which can be used if someone want's to reliably break on "throw" in optimized builds.
rdar://problem/66055678
