[caller-analysis] Implement findLocalApplySites and reimplement caller analysis on top of it #17971

gottesmm · 2018-07-15T23:19:13Z

This is a reimplementation of caller analysis on top of a new utility called findLocalApplySites. This utility works in spirit as an inverse operation to ApplySite::getCalleeOrigin(). At a high level starting at a specific function_ref, the utility traverses through the transitive use edges from the function_ref, looking through specific conversion insts and uses that it understands. All of the apply sites found this way are returned to the user and more importantly if there is an unknown use, a bool is set. As a result, the caller analysis (although not used anywhere beyond tests in this commit), can not be used to determine if the analysis can guarantee that all callers for a specific callee have been found.

rdar://41146023

gottesmm · 2018-07-15T23:19:20Z

@swift-ci smoke test

gottesmm · 2018-07-16T00:02:26Z

Made a mistake while splicing things... hmmm...

gottesmm · 2018-07-16T00:55:28Z

Yea... its some sort of memory error... Hmm... Compiling locally with asan...

gottesmm · 2018-07-16T18:31:57Z

Just discovered that we are not being complete with our notifyAdd/Delete notifications as I was tracking down the memory issue.

gottesmm · 2018-07-16T18:32:13Z

I am going to fix that in a different PR though.

atrick

This looks great. Most of my comments are about clearly communicating the intent and semantics and sticking with certain conventions so we can easily refer to the API in the future without deriving its meaning from the code.

atrick · 2018-07-16T17:11:50Z

include/swift/SIL/InstructionUtils.h

+  /// NOTE: We want to return true if escapes has any value. We want to return
+  /// true if we have any non-conservative information that the user should
+  /// process.
+  operator bool() const {


An operator bool overload should only be used for a result that has a completely obvious "has a valid value" or "has no value". A false result should mean that the analysis failed, so no information is available. i.e. it found an unrecognized SIL pattern. That seems to be the intention based on the comments, although that could be stated directly as: "Returns false for an invalid result. No assumptions can be made based an an invalid result".

However, the implementation seems to do something different. It apparently returns 'true' if an unknown use was found and 'false' if no uses were found. If that's really the desired semantics, then the comment needs to be rewritten and the name should be explicit, like foundUse or hasUseWithSideEffects (I started to say "hasNonTrivialUse", then realized we restrict the term "trivial" to ownership).

The reason that I put this is in was so that I could use an if scope, e.x.:

if (auto result = findLocalApplySites(fri)) {

Looks nice, right? But I get your point.

I don't get your point. I'm saying you should be able to do this:

if (auto result = findLocalApplySites(fri)) {

As long as your intention is that the if statement executes whenever the result is valid.

atrick · 2018-07-16T17:15:46Z

include/swift/SIL/InstructionUtils.h

+  ///
+  /// The none case is so that we can distinguish in between saying that a value
+  /// did escape and saying that we did not find any conservative information.
+  Optional<bool> escapes;


If there are no (nontrivial) uses of the function_ref, then I would expect this to contain a definite negative result (false). A missing result should indicate that the analysis cannot determine the result.

atrick · 2018-07-16T17:24:57Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h

-  llvm::DenseMap<SILFunction *, FunctionInfo> FuncInfos;
+  ///
+  /// We use a map vector to ensure that when we dump the state of the caller
+  /// analysis,


Comment trails off.

atrick · 2018-07-16T19:13:38Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h

+  /// indirectly. That is a separate query that is type system specific.
+  bool isDirectCallerSetComplete : 1;
+
+  CallerInfo()


A default ctor isn't necessary.

atrick · 2018-07-16T19:42:18Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h


-  /// The number of partial applied arguments of this function.
+  /// This is a special set vector that is an abuse as a performance


This comment appears to be a commentary on some other code. Is there some abuse happening? Do you mean optimization passes are using/abusing this information? Or does this just make the CallerAnalysis invalidation more efficient?

The comment should first state what calleeStates actually represents. I think this is simply the set of callees computed during analysis. If so, we should just say that. Maybe you're saying that this set should not be used for any purpose other than recomputing CallerInfo because it may be incomplete or out of date? If so, we should say that.

atrick · 2018-07-16T21:03:41Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h

+
+  /// Internal only way for getting a caller info. Will insert f if needed and
+  /// _WILL NOT_ preform any recomputation of the callgraph.
+  FunctionInfo &getOrInsertCallerInfo(SILFunction *f);


I keep getting confused by the name getOrInsertCallerInfo. I'm naturally expecting it to return a CallerInfo. I guess this should actually be getOrInsertFunctioInfo.

atrick · 2018-07-16T21:05:31Z

lib/SILOptimizer/Analysis/CallerAnalysis.cpp

+          // TODO: Make this more aggressive by considering
+          // final/visibility/etc.
+          calleeInfo.mayHaveIndirectCallers =
+              canBeCalledIndirectly(calleeFn->getRepresentation());


I find it confusing that this is handled while analyzing the call site. Shouldn't calleeInfo.mayHaveIndirectCallers be part of FunctionInfo initialization?

atrick · 2018-07-16T21:26:25Z

include/swift/SIL/InstructionUtils.h

+  /// Treat this function ref as escaping only if we found an actual user we
+  /// didn't understand. Do not treat it as escaping if we did not find any
+  /// users at all.
+  bool isEscaping() const { return escapes.getValueOr(false); }


The name mayEscape would match the semantics you have: null or false.

atrick · 2018-07-16T22:17:32Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h

  /// Returns true if this function has at least one caller.
-  bool hasCaller() const { return !Callers.empty(); }
+  bool hasCaller() const {


Should this be called hasDirectCaller?

atrick · 2018-07-16T22:27:45Z

lib/SILOptimizer/Analysis/CallerAnalysis.cpp

-    FunctionInfo &CalleeInfo = FuncInfos[Callee];
-    CalleeInfo.Callers.erase(F);
-    CalleeInfo.PartialAppliers.erase(F);
+void CallerAnalysis::invalidateExistingCalleeRelation(SILFunction *caller) {


I don't know what Existing implies here. If you don't want to refer to the computed callees simply as callees, then I would use the term "known callees" consistently:
calleeStates -> knownCallees
invalidateExistingCalleeRelation -> invalidateKnownCallees

gottesmm · 2018-08-07T19:18:17Z

Got in the functionality I needed to make sure that we get all callee relations in the short term (need to do a bit more work, but I can at least get htis in and get it out of the way).

gottesmm · 2018-08-07T19:32:47Z

@swift-ci smoke test macOS platform

gottesmm · 2018-08-07T19:32:56Z

@swift-ci smoke test macOS platform

gottesmm · 2018-08-07T19:33:03Z

@swift-ci smoke test macOS platform

gottesmm · 2018-08-07T19:33:09Z

@swift-ci smoke test osx platform

gottesmm · 2018-08-07T19:33:14Z

@swift-ci smoke test osx platform

gottesmm · 2018-08-07T19:33:38Z

@swift-ci smoke test OS X platform

gottesmm · 2018-08-07T19:33:48Z

@swift-ci asan test

gottesmm · 2018-08-07T20:06:40Z

Reproduced the crasher with ASAN.

gottesmm · 2018-08-18T20:01:35Z

@swift-ci test

gottesmm · 2018-08-18T20:01:38Z

@swift-ci test

gottesmm · 2018-08-18T20:01:42Z

@swift-ci test

gottesmm · 2018-08-18T20:01:47Z

@swift-ci test

gottesmm · 2018-08-18T20:01:52Z

@swift-ci test asan

swift-ci · 2018-08-18T20:02:12Z

Build failed
Swift Test OS X Platform
Git Sha - 744fa4faac2891eaa43999b1ea3ce9943d3b6847

gottesmm · 2018-08-18T20:02:14Z

@swift-ci test

gottesmm · 2018-08-18T20:02:20Z

@swift-ci test asan

gottesmm · 2018-08-18T20:04:49Z

@swift-ci smoke benchmark

swift-ci · 2018-08-18T20:26:25Z

Build failed
Swift Test Linux Platform
Git Sha - 285e301bd3b50172b4c701105c162b596165c93a

swift-ci · 2018-08-18T20:29:24Z

Build comment file:

Compilation-performance test failed

swift-ci · 2018-08-18T20:58:00Z

Build failed
Swift Test OS X Platform
Git Sha - 285e301bd3b50172b4c701105c162b596165c93a

swift-ci · 2018-08-18T21:22:48Z

Build comment file:

Build failed before running benchmark.

atrick

The code looks great. Please have a look at the comments below before merging.

atrick · 2018-08-19T00:44:52Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h

+/// an ask for information.
+///
+/// This laziness is implemented by the pass invalidating its internal state for
+/// a function f when receiving an invalidation message for f and then adding f


a function f when

I thought this was a typo. Use F like you do in other comments.

atrick · 2018-08-19T01:10:03Z

include/swift/SILOptimizer/Analysis/CallerAnalysis.h

+  ///
+  /// By storing the minimum number of partial applied arguments, we are able
+  /// to decide quickly if we are able to eliminate dead captured arguments.
+  Optional<unsigned> numPartialAppliedArguments;


Sorry to micro-optimize, but this is quadratic in the number of functions (x2 for MapVectors), so...

This creates a lot of fragmentation within the callerStates map, and the "Optional" doesn't help with clarity. I would use a bitfield for numPartialAppliedArguments initialized to zero, a separate bit for "seen partial apply", and assert that it doesn't overflow, which is good to check anyway.

atrick · 2018-08-19T02:01:31Z

lib/SILOptimizer/Analysis/CallerAnalysis.cpp

+      if (auto *fri = dyn_cast<FunctionRefInst>(&i)) {
+        // If we found any interesting information, update our internal state.
+        if (auto result = findLocalApplySites(fri)) {
+          findApplySites(callSitesThatMustBeVisited, callerFn, callerInfo, fri,


I don't think this code will build in Release mode. It looks like you might want to define a class/struct to hold the state required to process a caller function. That would solve the NDEBUG problem and address the findApplySite code smell where you're passing in a bunch of context that isn't specific to this invocation.

This utility works by taking in a function_ref and then traverses the transitive uses of the function_ref until it finds either a use it does not understand "escape" or an "apply" instruction. It returns a result structure that contains the final found applications and more importantly a bool telling the caller if we found any "escaping" uses. This is intended to be an inverse operation to ApplySite::getCalleeOrigin(). As such it has a bunch of assertions in it that check that the two stay in sync. rdar://41146023

gottesmm · 2018-08-22T00:42:45Z

@swift-ci test

gottesmm · 2018-08-22T00:42:50Z

@swift-ci asan test

gottesmm · 2018-08-22T00:42:54Z

@swift-ci test

gottesmm · 2018-08-22T00:42:56Z

@swift-ci test

swift-ci · 2018-08-22T01:08:40Z

Build failed
Swift Test Linux Platform
Git Sha - 858fa7c86bffc72231bad21d1a1b6c9e1aa78652

gottesmm · 2018-08-22T01:17:59Z

@swift-ci test linux platform

gottesmm · 2018-08-22T01:18:01Z

@swift-ci test linux platform

gottesmm · 2018-08-22T01:18:05Z

@swift-ci test linux platform

gottesmm · 2018-08-22T01:18:11Z

@swift-ci test linux platform

gottesmm · 2018-08-22T01:18:19Z

@swift-ci test Linux platform

swift-ci · 2018-08-22T01:44:27Z

Build failed
Swift Test Linux Platform
Git Sha - 71f6d68e44d9ef2f734b07eea0770b4229b72f11

…ites. Now the caller analysis can tell callers if it was able to find /all/ callers of a callee. NOTE: This does not change FSO itself yet. rdar://41146023

gottesmm · 2018-08-22T02:35:50Z

@swift-ci test

swift-ci · 2018-08-22T02:36:21Z

Build failed
Swift Test OS X Platform
Git Sha - 858fa7c86bffc72231bad21d1a1b6c9e1aa78652

gottesmm · 2018-08-22T02:37:21Z

The original linux failure was due to an issue with my result type being a move only type and not getting the NRVO behavior I wanted on Linux due to a conversion.

I fixed it by making the code "more NRVO"-able by eliminating the conversion and just working with an optional.

The second failure was b/c I forgot to do an emplace with that optional. = (. But at least on Linux it compiled!

I think this run will work.

swift-ci · 2018-08-22T02:37:55Z

Build failed
Swift Test Linux Platform
Git Sha - 71f6d68e44d9ef2f734b07eea0770b4229b72f11

gottesmm requested a review from atrick July 15, 2018 23:19

atrick reviewed Jul 16, 2018

View reviewed changes

gottesmm force-pushed the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch from 744fa4f to 6ec9dad Compare August 7, 2018 19:32

gottesmm closed this Aug 8, 2018

gottesmm deleted the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch August 8, 2018 18:11

gottesmm restored the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch August 8, 2018 18:15

gottesmm reopened this Aug 8, 2018

gottesmm force-pushed the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch from 6ec9dad to 285e301 Compare August 18, 2018 20:00

atrick approved these changes Aug 19, 2018

View reviewed changes

gottesmm force-pushed the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch 3 times, most recently from 1f7d373 to 858fa7c Compare August 22, 2018 00:42

gottesmm force-pushed the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch from 858fa7c to 71f6d68 Compare August 22, 2018 01:17

[caller-analysis] Reimplement CallerAnalysis ontop of findLocalApplyS…

03afdc5

…ites. Now the caller analysis can tell callers if it was able to find /all/ callers of a callee. NOTE: This does not change FSO itself yet. rdar://41146023

gottesmm force-pushed the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch from 71f6d68 to 03afdc5 Compare August 22, 2018 02:35

gottesmm merged commit f2a2376 into swiftlang:master Aug 22, 2018

gottesmm deleted the pr-3ce5268ef004895c1821c778f3e2af3e77a28509 branch August 22, 2018 06:02


		/// The number of partial applied arguments of this function.
		/// This is a special set vector that is an abuse as a performance

[caller-analysis] Implement findLocalApplySites and reimplement caller analysis on top of it #17971

[caller-analysis] Implement findLocalApplySites and reimplement caller analysis on top of it #17971

Uh oh!

Conversation

gottesmm commented Jul 15, 2018

Uh oh!

gottesmm commented Jul 15, 2018

Uh oh!

gottesmm commented Jul 16, 2018

Uh oh!

gottesmm commented Jul 16, 2018

Uh oh!

gottesmm commented Jul 16, 2018

Uh oh!

gottesmm commented Jul 16, 2018

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 7, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

swift-ci commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

Uh oh!

gottesmm commented Aug 18, 2018

atrick Jul 16, 2018 •

edited

Loading