In cea0f00598, `InstructionDeleter` began
deleting `load [take]` instructions. Analogous to how it creates a
`destroy_value` when deleting an instruction which consumes a value, in
the case of deleting a `load [take]` the `InstructionDeleter` inserts a
compensating `destroy_addr`.
Previously, `DeadCodeElimination` did not observe the creation of any
instructions created by the `InstructionDeleter`. In the case of the
newly created `destroy_addr`, DCE didn't mark that the `destroy_addr`
was live and so deleted it. The result was a leak.
Here, this is fixed by passing an `InstModCallbacks`--with an
`onCreateNewInst` implementation--down into `erasePhiArgument` that
eventually invokes the `InstructionDeleter`. When the
`InstructionDeleter` creates a new instruction, DCE marks it live.
The reason why I am doing this is that right now SelectionDAG seems to sink
certain llvm.dbg.addr to the end of block. By breaking the block, we ensure that
we initialize the debug value of values early enough. NOTE: We are relying on
code at -Onone not eliminating control flow which it does not today.
A few additional notes:
1. I also fixed a small issue where I was not setting the proper debug scope for
reinit debug_values. This came up when I was writing test cases for this commit.
So I just fixed it at the same time.
2. Since I am splitting blocks and I don't want to invalidate dominators/etc at
this point in the pipeline, I took advantage of the APIs ability to update
dominators/loopinfo at the same time.
3. I also expanded the tests in tree further by adding debug info tests for the
reinit copyable/addressonly cases in the multi-block case.
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);
}
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.
I reimplemented the original addNewEdgeValueToBranch to just call the new
overload with a default InstModCallbacks, so nothing changed and now we can plug
in callbacks to this utility!
Requested by gottesmm during review.
Update the variable naming conventions to lower-camel.
Run clang-format.
I'm sure I missed some local variables somewhere--this was a best
effort.
The XXOptUtils.h convention is already established and parallels
the SIL/XXUtils convention.
New:
- InstOptUtils.h
- CFGOptUtils.h
- BasicBlockOptUtils.h
- ValueLifetime.h
Removed:
- Local.h
- Two conflicting CFG.h files
This reorganization is helpful before I introduce more
utilities for block cloning similar to SinkAddressProjections.
Move the control flow utilies out of Local.h, which was an
unreadable, unprincipled mess. Rename it to InstOptUtils.h, and
confine it to small APIs for working with individual instructions.
These are the optimizer's additions to /SIL/InstUtils.h.
Rename CFG.h to CFGOptUtils.h and remove the one in /Analysis. Now
there is only SIL/CFG.h, resolving the naming conflict within the
swift project (this has always been a problem for source tools). Limit
this header to low-level APIs for working with branches and CFG edges.
Add BasicBlockOptUtils.h for block level transforms (it makes me sad
that I can't use BBOptUtils.h, but SIL already has
BasicBlockUtils.h). These are larger APIs for cloning or removing
whole blocks.
