Instead of caching alias results globally for the module, make AliasAnalysis a FunctionAnalysisBase which caches the alias results per function.
Why?
* So far the result caches could only grow. They were reset when they reached a certain size. This was not ideal. Now, they are invalidated whenever the function changes.
* It was not possible to actually invalidate an alias analysis result. This is required, for example in TempRValueOpt and TempLValueOpt (so far it was done manually with invalidateInstruction).
* Type based alias analysis results were also cached for the whole module, while it is actually dependent on the function, because it depends on the function's resilience expansion. This was a potential bug.
I also added a new PassManager API to directly get a function-base analysis:
getAnalysis(SILFunction *f)
The second change of this commit is the removal of the instruction-index indirection for the cache keys. Now the cache keys directly work on instruction pointers instead of instruction indices. This reduces the number of hash table lookups for a cache lookup from 3 to 1.
This indirection was needed to avoid dangling instruction pointers in the cache keys. But this is not needed anymore, because of the new delayed instruction deletion mechanism.
... with a fix for a non-assert build crash: I used the wrong ilist type for SlabList. This does not explain the crash, though. What I think happened here is that llvm miscompiled and put the llvm_unreachable from the Slab's deleteNode function unconditionally into the SILModule destructor.
Now by using simple_ilist, there is no need for a deleteNode at all.
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.
This disables a bunch of passes when ownership is enabled. This will allow me to
keep transparent functions in ossa and skip most of the performance pipeline without
being touched by passes that have not been updated for ownership.
This is important so that we can in -Onone code import transparent functions and
inline them into other ossa functions (you can't inline from ossa => non-ossa).
In the included, test case, the optimization was sinking
releases past is_escaping_closure.
Rewrite the isBarrier logic to be conservative and define the
mayCheckRefCount property in SIL/InstructionUtils. Properties that may
need to be updated when SIL changes belong there.
Note that it is particularly bad behavior if the presence of access
markers in the code cause miscompiles unrelated to access enforcement.
Fixes <rdar://problem/45846920> TestFoundation, TestProcess, closure
argument passed as @noescape to Objective-C has escaped.
* [SILOptimizer] Don't diagnose infinite recursion if a branch terminates the program
This patch augments the infinite recursion checker to not warn if a
branch terminates, but still warns if a branch calls into something with
@_semantics("programtermination_point"). This way, calling fatalError
doesn't disqualify you for the diagnostic, but calling exit does.
This also removes the warning workaround in the standard library, and
annotates the internal _assertionFailure functions as
programtermination_points, so they get this treatment too.
* Fix formatting in SILInstructions.cpp
* Re-add missing test
This patch augments the infinite recursion checker to not warn if a
branch terminates, but still warns if a branch calls into something with
`@_semantics("arc.programtermination_point")`. This way, calling `fatalError`
doesn't disqualify you for the diagnostic, but calling `exit` does.
This also removes the warning workaround in the standard library, and
annotates the internal _assertionFailure functions as
`programtermination_point`s, so they get this treatment too.
SIL passes were violating the existing invariant on non-cond-br
critical edges in several places. I fixed the places that I could
find. Wherever there was a post-pass to "clean up" critical edges, I
replaced it with a a call to verification that the critical edges
aren't broken in the first place.
We still need to eliminate critical edges entirely before enabling
ownership SIL.
The reason to do this is that otherwise, retain code motion can increase the
size of the IR by large amounts as it pushes tons and tons of retains/releases
into these sorts of blocks. One one test case, it increased the amount of raw
instructions by 2-3 orders of magnitude.
We used to implement this in ARCSequenceOpts when it did code motion. When code
motion was stripped out of ARCSequenceOpts and ARCCodeMotion was implemented,
this was not implemented for some reason yielding this regression.
NOTE: If one reads this PR there is "retain" trimming code as well as usage of
ProgramTerminationAnalysis. The reason why I implemented the code in this way is
that tThe program termination analysis allows us to avoid inserting retains at
all into fatal error blocks, but it can not handle cases where a fatal error
block has been merged into another block. The trimmer code handles this case and
potential cases where side-effect code is in the fatal error block. On the other
hand, if we just relied on the retain trimming code, we would be inserting a
huge amount of retains just to free them. That is really wasteful from a
performance standpoint given the amount of retains that we can insert here.
rdar://42347024
Moved the setting to the run section of arc-code motion.
In this case I think it is safe to do this, but calling a virtual function is
generally a code smell, so no reason to keep it in the code base unless we need
to.
Caught by the clang static analyzer.
introduce a common superclass, SILNode.
This is in preparation for allowing instructions to have multiple
results. It is also a somewhat more elegant representation for
instructions that have zero results. Instructions that are known
to have exactly one result inherit from a class, SingleValueInstruction,
that subclasses both ValueBase and SILInstruction. Some care must be
taken when working with SILNode pointers and testing for equality;
please see the comment on SILNode for more information.
A number of SIL passes needed to be updated in order to handle this
new distinction between SIL values and SIL instructions.
Note that the SIL parser is now stricter about not trying to assign
a result value from an instruction (like 'return' or 'strong_retain')
that does not produce any.
1) PostOrderAnalysis is not invalidated after splitting critical edges. This let the data flow solver omit new inserted blocks.
2) Handle infinite loops in the CFG correctly. So that we don’t insert random release instructions into such CFG pathes.
https://bugs.swift.org/browse/SR-5187
rdar://problem/32713742
Replace `NameOfType foo = dyn_cast<NameOfType>(bar)` with DRY version `auto foo = dyn_cast<NameOfType>(bar)`.
The DRY auto version is by far the dominant form already used in the repo, so this PR merely brings the exceptional cases (redundant repetition form) in line with the dominant form (auto form).
See the [C++ Core Guidelines](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es11-use-auto-to-avoid-redundant-repetition-of-type-names) for a general discussion on why to use `auto` to avoid redundant repetition of type names.
At some point, pass definitions were heavily macro-ized. Pass
descriptive names were added in two places. This is not only redundant
but a source of confusion. You could waste a lot of time grepping for
the wrong string. I removed all the getName() overrides which, at
around 90 passes, was a fairly significant amount of code bloat.
Any pass that we want to be able to invoke by name from a tool
(sil-opt) or pipeline plan *should* have unique type name, enum value,
commend-line string, and name string. I removed a comment about the
various inliner passes that contradicted that.
Side note: We should be consistent with the policy that a pass is
identified by its type. We have a couple passes, LICM and CSE, which
currently violate that convention.
In particular, support the following optimizations:
- owned-to-guaranteed
- dead argument elimination
Argument explosion is disabled for generics at the moment as it usually leads to a slower code.
Changes:
* Terminate all namespaces with the correct closing comment.
* Make sure argument names in comments match the corresponding parameter name.
* Remove redundant get() calls on smart pointers.
* Prefer using "override" or "final" instead of "virtual". Remove "virtual" where appropriate.
This was already done for getSuccessorBlocks() to distinguish getting successor
blocks from getting the full list of SILSuccessors via getSuccessors(). This
commit just makes all of the successor/predecessor code follow that naming
convention.
Some examples:
getSingleSuccessor() => getSingleSuccessorBlock().
isSuccessor() => isSuccessorBlock().
getPreds() => getPredecessorBlocks().
Really, IMO, we should consider renaming SILSuccessor to a more verbose name so
that it is clear that it is more of an internal detail of SILBasicBlock's
implementation rather than something that one should consider as apart of one's
mental model of the IR when one really wants to be thinking about predecessor
and successor blocks. But that is not what this commit is trying to change, it
is just trying to eliminate a bit of technical debt by making the naming
conventions here consistent.
delete it and recreate new one
This is a compilation time improvement
There are a few small modifications to the tests, as we try to create
different, but equivalent retain/release before even though we can reuse
the old ones.
rdar://28329689
