Extend AllocBoxToStack to handle apply #31974

meg-gupta · 2020-05-22T20:39:08Z

AllocBoxToStack analyzes the uses of boxes and promotes them to stack if it is safe to do so. Currently the analysis is limited to only a few known users including partial_apply.
With this change, the pass also analyzes apply users, where the callee is a local private function. The analysis is recursive and bound by a threshold.

Fixes rdar://59070139

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

gottesmm

Some quick notes. I want to look through it in more detail.

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

meg-gupta · 2020-05-29T03:49:04Z

@swift-ci test

meg-gupta · 2020-05-29T03:50:02Z

@swift-ci Please Test Source Compatibility

swift-ci · 2020-05-29T06:03:49Z

Build failed
Swift Test Linux Platform
Git Sha - 005d012ce0f4033bc92a6764a9a44076f36ec013

gottesmm

Some more comments around efficiency. I think I need to read it in source to understand the algorithmic change.

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

test/SILOptimizer/allocboxtostack_localapply.swift

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

private functions on specialization were being given shared linkage. Use swift::getSpecializeLinkage to correctly get the linkage for the specialized function based on the linkage of the original function.

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

meg-gupta · 2020-06-12T06:11:39Z

@swift-ci test

meg-gupta · 2020-06-12T06:12:01Z

@swift-ci Please Test Source Compatibility

swift-ci · 2020-06-12T06:14:25Z

Build failed
Swift Test Linux Platform
Git Sha - 005d012ce0f4033bc92a6764a9a44076f36ec013

swift-ci · 2020-06-12T06:14:28Z

Build failed
Swift Test OS X Platform
Git Sha - 005d012ce0f4033bc92a6764a9a44076f36ec013

AllocBoxToStack analyzes the uses of boxes and promotes them to stack if it is safe to do so. Currently the analysis is limited to only a few known users including partial_apply. With this change, the pass also analyzes apply users, where the callee is a local private function. The analysis is recursive and bound by a threshold. Fixes rdar://59070139

gottesmm

I have a few more comments but we can do them in a follow on commit. I think this code is beautiful. = ). The tests you added handled all the cases. Large +1 from me! = ).

gottesmm · 2020-06-18T02:40:04Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

 }

-static void rewritePartialApplies(AllocBoxToStackState &pass) {
-  llvm::DenseMap<PartialApplyInst *, ArgIndexList> IndexMap;
+static void rewriteApplies(AllocBoxToStackState &pass) {


As a follow on commit, can you rename this to something more descriptive like: rewriteApplySites? If you use Applies here it would mean an ApplyInst to me. But again, I don't want that to hold up this commit.

gottesmm · 2020-06-18T02:42:15Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

-                       AllocBoxToStackState &pass) {
-  auto *FRI = cast<FunctionRefInst>(PartialApply->getCallee());
-  assert(FRI && "Expected a direct partial_apply!");
+static SILInstruction *specializeApply(SILOptFunctionBuilder &FuncBuilder,


In a follow on commit, can you rename this to specializeApplySite?

gottesmm · 2020-06-18T02:54:31Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

-/// Specialize a partial_apply by promoting the parameters indicated by
+// While cloning during specialization, make sure apply instructions do not have
+// box arguments that need to be promoted.
+// This is an assertion in debug builds only. This should never be true because


In a follow on commit, can you add that the reason why this is true? My thought:

This is an assertion in debug builds only. The reason why this should never be true is that we have cloned our callees in DFS order meaning that any of our callees that had a promotable box will have already have been promoted away by the time this runs.

The part about those promotable boxes having already been handled is missing from the comment!

gottesmm · 2020-06-18T03:20:10Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

-/// true if this partial apply will not block our promoting the box.
-static bool checkPartialApplyBody(Operand *O) {
+/// true if this apply will not block our promoting the box.
+static bool checkLocalApplyBody(Operand *O,


In a follow on commit, can you give this a more descriptive name/improve the comments? The big thing here missing from the comment and from the name of the function is that we are /not/ only checking for blocking uses. Specifically, we are also using this to recursively specialize a chain of applies and that since we are visiting recursively, we add the operands (which are in different functions) to the list in DFS order due to the nature of stack recursion.

My hope is that I get that information from the name of this function/the comment. I have a similar issue with findUnexpectedBoxUse. With those names/descriptions it is confusing that you are seeding the algorithm.

gottesmm · 2020-06-18T03:20:21Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

-//===----------------------------------------------------------------------===//
+static llvm::cl::opt<unsigned> MaxLocalApplyRecurDepth(
+    "max-local-apply-recur-depth", llvm::cl::init(4),
+    llvm::cl::desc("Mac recursove depth for analyzing local functions"));


In a follow on commit: Mac => Max.

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

gottesmm · 2020-06-18T03:22:49Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

+      case ApplySiteKind::ApplyInst:
+      case ApplySiteKind::BeginApplyInst:
+      case ApplySiteKind::TryApplyInst:
+        if (isEligibleApply(Apply) &&


Question, shouldn't isEligibleApply really be isOptimizableLocalApply? Anyways, just a random thought. Feel free to change it in a follow on commit if it makes sense to you. Otherwise, I am ok with this.

gottesmm · 2020-06-18T03:24:45Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

@@ -364,12 +422,13 @@ static InFlightDiagnostic diagnose(ASTContext &Context, SourceLoc loc,
 /// canPromoteAllocBox - Can we promote this alloc_box to an alloc_stack?
 static bool canPromoteAllocBox(AllocBoxInst *ABI,
                               SmallVectorImpl<Operand *> &PromotedOperands) {
+  SmallPtrSet<SILFunction *, 8> VisitedCallees;
  // Scan all of the uses of the address of the box to see if any
  // disqualifies the box from being promoted to the stack.
  if (auto *User = findUnexpectedBoxUse(ABI,


In a follow on commit, can you add a comment here saying that this initializes PromotedOperands from our recursive dfs walk of the call graph. I just want that to be very clear. Right now you need to read the code to understand that. I shouldn't have to read the code.

gottesmm

Just saw you did an update rather than follow on commits. Took a quick look. All of this can be done in a follow on commit as well.

gottesmm · 2020-06-18T05:32:08Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

-findUnexpectedBoxUse(SILValue Box, bool examinePartialApply,
-                     bool inAppliedFunction,
-                     SmallVectorImpl<Operand *> &PromotedOperands) {
+findUnexpectedBoxUseInDFSOrder(SILValue Box, bool inAppliedFunction,


My thought:

recursivelyFindBoxOperandsPromotableToAddress

The reason I am pushing on this is that the important thing here is computing PromotedOperands. That we return the unexpected use is just a side-effect. Your thoughts?

That being said. The comment is beautiful and is exactly what I hoped for!

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

gottesmm · 2020-06-18T05:34:09Z

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

+    // AllocBoxStack opt promotes boxes passed to a chain of applies when it is
+    // safe to do so. All such applies have to be specialized to take pointer
+    // arguments instead of box arguments This has to be done in dfs order.
+    // PromotedOperands is already in dfs order. Build AppliesToSpecialize in


I would make it clear that is it already in dfs order due to how it is constructed by find**. Someone reading this code needs a back reference to the code above.

meg-gupta · 2020-06-18T06:32:00Z

@swift-ci test

meg-gupta · 2020-06-18T06:32:47Z

@swift-ci test source compatibility

swift-ci · 2020-06-18T07:57:48Z

Build failed
Swift Test OS X Platform
Git Sha - 236632f

meg-gupta · 2020-06-18T18:16:56Z

@swift-ci smoke test

meg-gupta · 2020-06-19T18:03:35Z

@swift-ci test

swift-ci · 2020-06-19T18:04:01Z

Build failed
Swift Test Linux Platform
Git Sha - 236632f

meg-gupta · 2020-06-19T18:04:43Z

@swift-ci Please Test Source Compatibility

swift-ci · 2020-06-19T18:06:54Z

Build failed
Swift Test OS X Platform
Git Sha - 236632f

meg-gupta requested a review from gottesmm May 22, 2020 20:39

meg-gupta force-pushed the localapply branch from a9625d6 to 1653324 Compare May 22, 2020 21:47

gottesmm reviewed May 26, 2020

View reviewed changes

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Outdated Show resolved Hide resolved

gottesmm reviewed May 26, 2020

View reviewed changes

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Outdated Show resolved Hide resolved

meg-gupta force-pushed the localapply branch from 1653324 to 005d012 Compare May 29, 2020 01:00

meg-gupta force-pushed the localapply branch from 005d012 to 3ab3ab3 Compare June 1, 2020 18:15

gottesmm reviewed Jun 10, 2020

View reviewed changes

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Outdated Show resolved Hide resolved

gottesmm reviewed Jun 12, 2020

View reviewed changes

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Outdated Show resolved Hide resolved

Don't always give shared linkage to spl functions

de9b696

private functions on specialization were being given shared linkage. Use swift::getSpecializeLinkage to correctly get the linkage for the specialized function based on the linkage of the original function.

meg-gupta force-pushed the localapply branch from c600e61 to 1436524 Compare June 12, 2020 04:52

meg-gupta commented Jun 12, 2020

View reviewed changes

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp Show resolved Hide resolved

meg-gupta force-pushed the localapply branch from 1436524 to cfe70c2 Compare June 12, 2020 06:11

meg-gupta force-pushed the localapply branch from cfe70c2 to 6bee966 Compare June 12, 2020 16:53

meg-gupta force-pushed the localapply branch from 645cef6 to 3745f73 Compare June 17, 2020 01:08

gottesmm approved these changes Jun 18, 2020

View reviewed changes

Address review comments

236632f

meg-gupta force-pushed the localapply branch from 6e9b290 to 236632f Compare June 18, 2020 05:11

gottesmm reviewed Jun 18, 2020

View reviewed changes

Address review comments and test fixes

12b8885

meg-gupta merged commit 1d4f617 into swiftlang:master Jun 22, 2020

Extend AllocBoxToStack to handle apply #31974

Extend AllocBoxToStack to handle apply #31974

Uh oh!

Conversation

meg-gupta commented May 22, 2020

Uh oh!

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meg-gupta commented May 29, 2020

Uh oh!

meg-gupta commented May 29, 2020

Uh oh!

swift-ci commented May 29, 2020

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meg-gupta commented Jun 12, 2020

Uh oh!

meg-gupta commented Jun 12, 2020

Uh oh!

swift-ci commented Jun 12, 2020

Uh oh!

swift-ci commented Jun 12, 2020

Uh oh!

gottesmm left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Jun 18, 2020

Uh oh!

meg-gupta commented Jun 18, 2020

Uh oh!

swift-ci commented Jun 18, 2020

Uh oh!

meg-gupta commented Jun 18, 2020

Uh oh!

meg-gupta commented Jun 19, 2020

Uh oh!

swift-ci commented Jun 19, 2020