TLDR: I fixed a whole in the assembly-vision opt-remark pass where we were not
emitting a remark for end of scope instructions at the beginning of blocks. Now
all of these instructions (strong_release, end_access) should always reliably
have a remark emitted for them.
----
I think that this is a pragmatic first solution to the problem of
strong_release, release_value being the first instruction of a block. For those
who are unaware, this issue is that for a long time we have searched backwards
first for "end of scope" like instructions. This then allows us to identify the
"end of scope" instruction as happening at the end of the previous statement
which is where the developer thinks it should be:
```
var global: Klass
func bar() -> @owned Klass { global }
func foo() {
// We want the remark for the
bar() // expected-remark {{retain}}
}
```
This makes sense since we want to show end of scope instructions as being
applied to the earlier code whose scope it is ending. We can be clear that it is
at the end of the statement by placing the carrot on the end of statement
SourceLoc so there isn't any confusion upon whether or not
That generally has delivered nice looking results, but what if our release is
the first instruction in the block? In that case, we do not have any instruction
that we can immediately use, so traditionally we just gave up and didn't emit
anything. This is not an acceptable solution! We should be able to emit
something for every retain/release in the program if we want users to be able to
rely upon this! Thus we need to be able to get source location information from
somewhere around
First before we begin, my approach here is informed by my seeing over time that
the optimizer does a pretty good job of not breaking SourceLoc info for
terminators.
With that in mind, there are two possible approaches here: using the terminator
from the previous block and searching forward at worst taking the SourceLoc of
the current block's terminator (or earlier if we find a good SourceLoc). I
wasn't sure what the correct thing to do was at the time so I didn't fix the
issue. After some thought, I realized that the correct solution is to if we fail
a backwards search, search forwards. The reason why is that since our remarks
runs late in the optimization pipeline, there is a very high likelihood that if
we aren't folded into our previous block that there is a true need in the
program for conditional control flow here. We want to avoid placing the release
out of such pieces of code since it is misleading to the user:
```
In this example there is a release inside the case for .x but none for .y. In
that case it is possible that we get a release for .f since payload is passed in
at +1 at SILGen time. In such a case, using the terminator of the previous block
would mean that we would have the release be marked as on payload instead of
inside the case of .x. By using the terminator of the releases block, we can
switch payload {
case let .x(f):
...
case let .y:
...
}
```
So using the terminator from the previous block would be
misleading to the user. Instead it is better to pick a location after the release that way
we know at least the instruction we are inferring from must in some sense be
With this fix, we should not emit locations for all retains, releases. We may
not identify a good source loc for all of them, but we will identify them.
optimization pipeline, if our block was not folded into the previous block there
is a very high liklihood that there is some sort of conditional control flow
that is truly necessary in the program. If we
this
generally implies that there is a real side effect in the program that is
requiring conditional code execution (since the optimizer would have folded it).
The reason why is that we are at least going to hit a terminator or a
side-effect having instruction that generally have debug info preserved by the
optimizer.
For example, now we get the following diagnostic on globals:
public func getGlobal() -> Klass {
return global // expected-remark @:5 {{retain of type 'Klass'}}
// expected-note @-5:12 {{of 'global'}}
+ // expected-remark @-2:12 {{begin exclusive access to value of type 'Klass'}}
+ // expected-note @-7:12 {{of 'global'}}
+ // expected-remark @-4 {{end exclusive access to value of type 'Klass'}}
+ // expected-note @-9:12 {{of 'global'}}
+
}
and for classes when we can't eliminate the access:
+func simpleInOut() -> Klass {
+ let x = Klass() // expected-remark @:13 {{heap allocated ref of type 'Klass'}}
+ // expected-note @-1:9 {{of 'x'}}
+ simpleInOutUser(&x.next) // expected-remark @:5 {{begin exclusive access to value of type 'Optional<Klass>'}}
+ // expected-note @-3:9 {{of 'x.next'}}
+ // expected-remark @-2:28 {{end exclusive access to value of type 'Optional<Klass>'}}
+ // expected-note @-5:9 {{of 'x.next'}}
+ return x
+}
The reason why is that due to ARCCodeMotion, they could have moved enough that
the SourceLoc on them is incorrect. That is why you can see in the tests that I
had to update, I am moving the retain to the return statement from the body of
the function since the retain was now right before the return.
I also went in and cleaned up the logic here a little bit. What we do now is
that we have a notion of instructions that we /always/ infer SourceLocs for (rr)
and ones that if we have a valid non-inlined location we use (e.x.:
allocations).
This mucked a little bit with my ability to run SIL tests since the SIL tests
were relying on this not happening to rr so that we would emit remarks on the rr
instructions themselves. I added an option that disables the always infer
behavior for this test.
That being said at this point to me it seems like the SourceLoc inference stuff
is really tied to OptRemarkGenerator and I am going to see if I can move it to
there. But that is for a future commit on another day.
Specifically, now we properly identify the SILLocation for the final inlined
call site of a Source Location and put the remark there instead of in the
callee.
More specifically, if one wants to force emit /all/ opt-remarks on a function, mark it with:
```
@_semantics("optremark")
```
If one wants to emit opt-remarks only for a specific SIL pass (like lets say
sil-opt-remark-gen), one can write:
```
@_semantics("optremark.sil-opt-remark-gen")
```
I made the pattern matching strict so if you just put in a '.' or add additional
suffixes, it will not pattern match. I think that this is sufficient for a
prototyping tool.
This is useful if one wants to play around with opt-remarks when optimizing code
in Xcode or any IDE that can use serialized diagnostics.
I noticed that I kept on needing to hack on this as I added stuff to
OptRemarkGenerator and felt the need to tie it even closer to
OptRemarkGenerator. As I began to think about it, this is really something
specific to that algorithm, so it really doesn't make sense to have it as part
of the general SIL library.
This is a NFC refactoring.
In all of these cases, we already had a SILFunction and were just grabbing its
SILModule instead of passing it in. So this is just an NFC change.
The reason why I am doing this is so that I can force emit opt-remarks on
functions with the semantics attribute "optremark", so I need to be able to
access the SILFunction in the optimization remark infrastructure.
This is just an initial implementation and doesn't handle all cases. This works
by grabbing the rc-identity root of a retain/release operation and then seeing:
1. If it is an argument, we use the ValueDecl of the argument.
2. If it is an instruction result, we try to infer the ValueDecl by looking for debug_value uses.
3. In the future, we should add support for globals and looking at addresses.
I may do 3 since it is important to have support for that due to inout objects.
This is useful if one wants to emit a note explanation (in my case that we
couldn't find any values) but do not have any other source loc but the original
remark.
I did this by adding a kind to Arguments and making it so we have a kind that
emits as a separate NOTE when we emit diagnostics, but is emitted as a normal
argument when emitting to the bitstream.
In order to test this, I implemented a small source loc inference routine for
instructions without valid SILLocations. This is an optional nob that the
opt-remark writer can optionally enable on a per remark basis. The current
behaviors are just forward/backward scans in the same basic block. If we scan
forwards, if we find a valid SourceLoc, we just use ethat. If we are scanning
backwards, instead we grab the SourceRange and if it is valid use the end source
range of the given instruction. This seems to give a good result for retain
(forward scan) and release (backward scan).
The specific reason that I did that is that my test case for this are
retain/release operations. Often times these operations due to code-motion are
moved around (and rightly to prevent user confusion) given by optimizations auto
generated SIL locations. Since that is the test case I am using, to test this I
needed said engine.
Specifically, I split it into 3 initial categories: IR, Utils, Verifier. I just
did this quickly, we can always split it more later if we want.
I followed the model that we use in SILOptimizer: ./lib/SIL/CMakeLists.txt vends
a macro (sil_register_sources) to the sub-folders that register the sources of
the subdirectory with a global state variable that ./lib/SIL/CMakeLists.txt
defines. Then after including those subdirs, the parent cmake declares the SIL
library. So the output is the same, but we have the flexibility of having
subdirectories to categorize source files.
