[Optimize] Move dead partial apply elimination to mandatory combine #28536

zoecarver · 2019-12-03T17:34:24Z

This patch moves the removal of dead partial applies to mandatory combine (from mandatory inlining). SourceKit doesn't run mandatory combine but, it does run mandatory inlining, so the more we can move out of mandatory inlining, the better.

refs #28484 #28394 #28396
cc @gottesmm

zoecarver · 2019-12-10T16:12:32Z

@gottesmm friendly ping. Does the general idea of this patch look good to you?

slavapestov

Are there any tests that exercise the old code? If not, do you mind adding one?

slavapestov · 2019-12-11T05:20:21Z

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

+  // Remove the stack dealloc, then the partial apply, then the function ref.
+  instModCallbacks.deleteInst(user->getUser());
+  instModCallbacks.deleteInst(i);
+  instModCallbacks.deleteInst(cast<FunctionRefInst>(i->getCallee()));


It's unlikely but possible that the function_ref has other uses, so I'm not sure it's quite safe to remove it.

Good point. I'll fix this.

slavapestov · 2019-12-11T05:22:03Z

@swift-ci Please smoke test

zoecarver · 2019-12-11T05:26:28Z

@slavapestov this should mostly be a NFC. So, no tests should need to change. Do you mean I should add a test that only runs mandatory inlining (to make sure the closure isn't optimized)?

slavapestov · 2019-12-17T18:44:16Z

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

+  instModCallbacks.deleteInst(i);
+  // If the only use of the function_ref is us, then remove it.
+  if (i->getCallee()->getSingleUse()->getUser() == i)
+    instModCallbacks.deleteInst(cast<FunctionRefInst>(i->getCallee()));


I don't think you can guarantee that the callee is always going to be a function_ref. Can you add an isa check to make sure?

slavapestov · 2019-12-17T18:44:53Z

It's up to you if you want to add a SIL test that only runs mandatory combining or not (do please fix the unconditional cast though)

slavapestov · 2019-12-17T18:45:00Z

@swift-ci Please smoke test

zoecarver · 2019-12-31T19:31:40Z

test/SILOptimizer/mandatory_inlining_and_combine.sil

+  return %5 : $Builtin.Int8
+}
+
+sil shared [transparent] @identity_closure : $@convention(thin) (Builtin.Int8) -> Builtin.Int8 {


I'm ignoring the fact that this function no longer gets removed because it seems inline with the update in #28484

zoecarver · 2019-12-31T19:32:23Z

test/SILOptimizer/mandatory_inlining.sil

@@ -1088,7 +1012,7 @@ bb0(%0 : $C):
 // CHECK:   [[F:%.*]] = function_ref @use_c
 // CHECK:   apply [[F]]([[ARG]])
 // CHECK:   strong_release [[ARG]]
-// CHECK:   strong_release [[ARG]]
+// TODO: why is this broken?


I need to figure out why there are too few strong_releases here. Any ideas?

…tils

zoecarver · 2020-01-02T23:18:39Z

test/SILOptimizer/mandatory_inlining_and_combine.sil

+// CHECK-NEXT:  br [[MERGE_BB]]
+//
+// CHECK: [[MERGE_BB]]:
+// TODO: strong_release [[ARG]]


I was able to fix the other strong_release todos but I'm still looking into the cause of this one.

zoecarver · 2020-01-02T23:25:18Z

@slavapestov I've added mandatory_inlining_and_combine.sil to test that mandatory inlining and mandatory combine each do the "right" thing.

I had to update this patch quite a bit to cover all the closure cleanup done in mandatory inlining, now it should cover all the cases that it used to.

mandatory_inlining_ownership2.sil had a lot of tests that now required both a mandatory inlining and mandatory combine pass. Instead of moving them to mandatory_inlining_and_combine.sil I added a RUN comment that runs both passes. If you'd prefer, I'm happy to move it into mandatory_inlining_and_combine.sil (no sure which is best).

…o that cleanupLoadedCalleeValue can work properly (and other small fixes)

… combine

zoecarver · 2020-02-22T19:42:02Z

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -1449,7 +1449,8 @@ static bool keepArgsOfPartialApplyAlive(PartialApplyInst *pai,

  ValueLifetimeAnalysis::Frontier partialApplyFrontier;
  if (!vla.computeFrontier(partialApplyFrontier,
-                           ValueLifetimeAnalysis::DontModifyCFG)) {
+                           ValueLifetimeAnalysis::DontModifyCFG) ||
+      partialApplyFrontier.empty()) {


If partialApplyFrontier is empty (and we don't return), we will create an alloc_stack and copy_addr but no dealloc_stack.

zoecarver · 2020-02-22T19:47:31Z

@slavapestov and @gottesmm friendly ping. I've made sure that all the tests will pass after this patch is applied. Some of the tests appear to have been deleted but that either reflects an increase in deleted partial_applys or they have been moved elsewhere (such as from mandatory_inlining.sil to mandatory_inlining_and_combine.sil).

I realize this is a large patch so it may take a while to review. Let me know if you have any questions.

zoecarver · 2020-03-03T19:44:19Z

@swift-ci please test.

zoecarver · 2020-03-14T19:32:18Z

@swift-ci please test.

gottesmm

Some quick comments. I am going to want to relook at this after you make improvements/what we talked about over chat.

gottesmm · 2020-03-14T22:07:06Z

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -1943,3 +1944,102 @@ AbstractFunctionDecl *swift::getBaseMethod(AbstractFunctionDecl *FD) {
  }
  return FD;
 }
+
+SILValue swift::stripCopiesAndBorrows(SILValue v) {
+  while (isa<CopyValueInst>(v) || isa<BeginBorrowInst>(v)) {


I think we already have a routine like this. I would look around.

gottesmm · 2020-03-14T22:14:58Z

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

@@ -201,11 +203,6 @@ bool MandatoryCombiner::doOneIteration(SILFunction &function,
      );
    }

-    for (SILInstruction *instruction : instructionsPendingDeletion) {


Why is this removed?

gottesmm · 2020-03-14T22:15:47Z

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

+    }
+  }
+
+  if (auto *convFunc = dyn_cast<ConvertFunctionInst>(val)) {


This is not needed. Should happen automatically when we go back around.

gottesmm · 2020-03-14T22:25:08Z

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

+      }
+    }
+  } else if (auto *tttf = dyn_cast<ThinToThickFunctionInst>(src)) {
+    if (tryDeleteDeadClosure(tttf, instModCallbacks)) {


Most of the optimizations here should not be on LoadInst. You aren't removing load inst anywhere! Instead, all of these should be on the actual instructions you are removing (i.e. ThinToThick, Partial, Etc).

* remove mand inlining changes * update combiner to be more like a "combiner" * delete trivially dead instructions * promote loads * don't delete instructions in the callback

zoecarver · 2020-03-15T22:51:24Z

I'll make all these changes as their own (smaller, more focused) PRs.

zoecarver added 3 commits December 1, 2019 16:36

Move partial_apply closure cleanup into mandatory combine

e069f5f

Add comments

112999f

Format

1ac0a9a

slavapestov reviewed Dec 11, 2019

View reviewed changes

zoecarver added 2 commits December 16, 2019 19:49

Merge branch 'master' into optimize/dead-closure-in-mand-combine

f354e25

Fix failing tests

833ca19

slavapestov reviewed Dec 17, 2019

View reviewed changes

zoecarver added 3 commits December 29, 2019 18:31

Merge branch 'master' into optimize/dead-closure-in-mand-combine

7805727

Fix mandatory_inlining.sil

35652ec

Rename tests

b7eef42

zoecarver commented Dec 31, 2019

View reviewed changes

zoecarver added 4 commits January 1, 2020 18:28

stash: add load visitor and refactor

fd55891

Use lambda to remove instructions

3824e86

Move stripCopiesAndBorrows and cleanupLoadedCalleeValue into InstOptU…

29e35da

…tils

Fix crash and remove todos

a383b76

zoecarver commented Jan 2, 2020

View reviewed changes

Remove unused ClosureCleanup from mandatory inlining

e6543b5

zoecarver requested a review from slavapestov January 2, 2020 23:32

zoecarver added 5 commits February 17, 2020 06:58

Merge branch 'master' into optimize/dead-closure-in-mand-combine

503cbc0

Merge branch 'master' into optimize/dead-closure-in-mand-combine

d8c13c2

Update instruction deleter callback to actually erease instructions s…

7fe24f0

…o that cleanupLoadedCalleeValue can work properly (and other small fixes)

Update mand inline sil tests after breaking closure cleanup into mand…

ac789a1

… combine

Removed unused member: tryRemoveUnused

3e08baa

zoecarver added 2 commits February 22, 2020 11:38

Fix remaining tests

25d99d7

Run formatter over code

9607682

zoecarver commented Feb 22, 2020

View reviewed changes

Fix still failing tests

4e9ad8a

Merge branch 'master' into optimize/dead-closure-in-mand-combine

006861d

gottesmm reviewed Mar 14, 2020

View reviewed changes

zoecarver added 2 commits March 15, 2020 15:44

Update based on review:

5fe078a

* remove mand inlining changes * update combiner to be more like a "combiner" * delete trivially dead instructions * promote loads * don't delete instructions in the callback

Reset tests / mand inline changes to master

f454769

zoecarver closed this Mar 15, 2020

This was referenced Mar 18, 2020

[opt] load promotion in mandatory combine #30463

Closed

[opt] dead thin to thick in mand combine #30475

Open

[Optimize] Move dead partial apply elimination to mandatory combine #28536

[Optimize] Move dead partial apply elimination to mandatory combine #28536

Uh oh!

Conversation

zoecarver commented Dec 3, 2019

Uh oh!

zoecarver commented Dec 10, 2019

Uh oh!

slavapestov 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!

slavapestov commented Dec 11, 2019

Uh oh!

zoecarver commented Dec 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Dec 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver Dec 31, 2019 • 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!

zoecarver commented Jan 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Feb 22, 2020

Uh oh!

zoecarver commented Mar 3, 2020

Uh oh!

zoecarver commented Mar 14, 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!

zoecarver commented Mar 15, 2020

Uh oh!

Uh oh!

slavapestov commented Dec 17, 2019 •

edited

Loading

zoecarver Dec 31, 2019 •

edited

Loading