Optimize dead private global variables #28324

zoecarver · 2019-11-18T17:08:16Z

This optimization removes private global variables when the only use is an allocation and a store instruction. To make sure I didn't remove used variables, I updated SILGlobalOpt to collect instructions from all functions. Then, it bails out when trying to optimize, if it finds an instruction with a parent function that shouldn't be optimized.

Resolves SR-6597.

zoecarver · 2019-11-18T17:08:29Z

cc @eeckstein

slavapestov

This looks great. I have some optional suggestions for further generalizations.

lib/SILOptimizer/IPO/GlobalOpt.cpp

test/SILOptimizer/remove_unused_global_vars.swift

lib/SILOptimizer/IPO/GlobalOpt.cpp

zoecarver · 2019-11-20T04:59:28Z

I moved the collection of instructions / global variables into a collect method and added a reset method. After I erase instructions, I trigger reset then collect so that the data is up to date, and the subsequent passes can successfully optimize the code. If there is a way to trigger a re-run of the pass, that might be cleaner. Feel free to suggest alternatives.

I'll finish writing tests tomorrow.

zoecarver · 2019-11-24T21:03:39Z

Any other review comments (or comments I missed)?

slavapestov

Sorry for the delay in responding to this. Thanksgiving week was a holiday for us.

lib/SILOptimizer/IPO/GlobalOpt.cpp

slavapestov · 2019-12-03T19:43:47Z

lib/SILOptimizer/IPO/GlobalOpt.cpp

+    addr->eraseFromParent();
+  }
+
+  reset();


I worry that calling collect() every time something changes is going to turn this pass into an O(n^2) algorithm. Is there a way to build a list of all instructions to be deleted, and then delete them at the end, and finally reset()?

Before deleting globals, I need to recollect everything in order to figure out what's now-dead. This will make it an O(n2) -- is that OK?

That's OK (and it answers one of my questions in the latest review) but my guess would be that it should be possible to determine in one pass both which instructions to delete, and which SIL globals become dead as a result.

Yes, see my comment.

test/SILOptimizer/remove_unused_global_vars_wmo.swift

slavapestov · 2019-12-03T19:45:37Z

@swift-ci Please smoke test

zoecarver · 2019-12-04T16:20:30Z

@slavapestov thanks for the review! I'll resolve your comment and look into the build failure.

zoecarver · 2019-12-06T19:15:03Z

The build failure occurred because a) I didn't exit early in tryRemoveGlobalAddr and b) because I didn't check GlobalVarSkipProcessing.

Also, I've updated all uses of .count(key) on a map of vectors to now be [key].size() because we no longer re-collect between every "optimization," so those elements may exist as empty vectors.

… in the global accessor function

zoecarver · 2019-12-14T02:42:30Z

@slavapestov I've addressed your comments and fixed some of the tests. While debugging one of the tests, I discovered that tryRemoveGlobalAlloc didn't check isSafeToRemove which meant that sometimes globals would be used but not allocated. I suspect that is the cause of most (if not all) of the miscompiles (which would explain why the swift program crashed and not the compiler).

I'll try to run all the tests tonight and fix the remaining failing tests.

zoecarver · 2019-12-14T18:35:45Z

lib/SILOptimizer/IPO/GlobalOpt.cpp

@@ -20,6 +20,7 @@
 #include "swift/SIL/SILCloner.h"
 #include "swift/SIL/SILGlobalVariable.h"
 #include "swift/SIL/SILInstruction.h"
+#include "swift/SIL/SILInstructionWorklist.h"


This is no longer needed.

slavapestov · 2019-12-17T18:37:26Z

@swift-ci Please test

slavapestov · 2019-12-17T18:37:31Z

@swift-ci Please test source compatibility

slavapestov · 2019-12-17T18:37:41Z

@swift-ci Please benchmark

swift-ci · 2019-12-17T18:39:38Z

Build failed
Swift Test Linux Platform
Git Sha - 2070821

swift-ci · 2019-12-17T18:42:50Z

Build failed
Swift Test OS X Platform
Git Sha - 2070821

swift-ci · 2019-12-17T19:02:18Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	4334	4691	+8.2%	0.92x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
DataAppendDataLargeToLarge	25800	38000	+47.3%	0.68x (?)
DropWhileCountableRange	15	17	+13.3%	0.88x (?)

Improvement	OLD	NEW	DELTA	RATIO
DropFirstCountableRangeLazy	17	14	-17.6%	1.21x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
ArrayAppendGenericStructs	630	1530	+142.9%	0.41x (?)

Improvement	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStubToNSDateRef	2540	2080	-18.1%	1.22x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

zoecarver · 2019-12-18T01:12:11Z

It looks like this patch caused a regression in static_strings.swift does that make sense or am I misdiagnosing the issue?

theblixguy · 2019-12-18T01:29:10Z

Maybe... it could be just that the test needs to be updated because the global is optimised out? The test is not supported on macOS so it was skipped.

(Also, do you mind squashing your commits? Patch has grown to nearly 30 commits).

eeckstein · 2019-12-18T08:45:45Z

@zoecarver it looks like a real regression. Can you please investigate this before merging?
@theblixguy The test passes on macOS, it's not disabled.

theblixguy · 2019-12-18T12:24:30Z

Oh, CI logs said “UNSUPPORTED” so I assumed it was skipped.

eeckstein · 2019-12-18T14:59:38Z

Wait, you are right. This test requires the stdlib to be built in no-assert mode. I think we are doing this only on linux. So it's in fact not running on our macOS CI bot. But it's not disabled for macOS in general.

zoecarver · 2019-12-18T16:22:12Z

Alrighty, this will be fun to debug :) I suspect it has to do with the modified collection of globals.

@theblixguy I'll rebase after I fix the regression.

zoecarver · 2019-12-24T23:41:47Z

test/SILOptimizer/static_strings.swift

@@ -5,7 +5,7 @@
 // RUN: %target-build-swift -O -module-name=test %s -o %t/a.out
 // RUN: %target-run %t/a.out | %FileCheck %s -check-prefix=CHECK-OUTPUT

-// REQUIRES: executable_test,swift_stdlib_no_asserts,optimized_stdlib
+// REQUIRES: executable_test,swift_stdlib_no_asserts


@eeckstein git blame showed that you were the one who added this requirement. The test passes without optimized_stdlib do you think this is an OK change?

zoecarver · 2019-12-24T23:43:29Z

I finally was able to reproduce/fix the issue. Thanks for the patience. Could someone re-trigger the bots?

slavapestov · 2020-01-02T20:43:56Z

Sorry, we were all out on vacation, again.

@swift-ci Please smoke test

slavapestov · 2020-01-02T20:44:19Z

@swift-ci Please smoke test

zoecarver · 2020-01-10T16:12:48Z

Friendly ping. Does this need any more reviewing or can it be merged?

zoecarver added 2 commits November 17, 2019 20:17

Add optimization to remove unused global private variables

9cd11a6

Refactor and add tests

f52a6f3

slavapestov reviewed Nov 18, 2019

View reviewed changes

slavapestov requested a review from eeckstein November 18, 2019 19:21

Cleanup

0e389ce

zoecarver commented Nov 20, 2019

View reviewed changes

lib/SILOptimizer/IPO/GlobalOpt.cpp Outdated Show resolved Hide resolved

zoecarver requested a review from slavapestov November 20, 2019 02:06

zoecarver commented Nov 20, 2019

View reviewed changes

lib/SILOptimizer/IPO/GlobalOpt.cpp Outdated Show resolved Hide resolved

Move collection into collect method

4dcbc42

Add tests

2070821

slavapestov reviewed Dec 3, 2019

View reviewed changes

zoecarver added 2 commits December 3, 2019 11:58

Merge branch 'master' into optimize/dead-global-vars

e470ed0

Merge branch 'master' into optimize/dead-global-vars

e73f027

zoecarver added 4 commits December 5, 2019 12:51

Create list of instructions to delete

73af502

Rename GlobalAccess to GlobalAccesses

e713ead

Resolve build failure

a8ad156

add helper function: canBeUsedOrChangedExternally

798eea8

zoecarver added 7 commits December 6, 2019 11:15

Check size of element, not if the element exists

95afdd0

Remove duplicate test and fix formatting

c11a971

Merge branch 'master' into optimize/dead-global-vars

6819c28

stash

179652c

Fix invalid global addr being collected

b366e9b

Add extra safety check before removing globals and add accessors even…

42a7b65

… in the global accessor function

Check that the only uses of a global addrs are stores

97f1b4e

zoecarver added 3 commits December 13, 2019 17:56

Remove double-collect

b81e47b

Fix issues with worklist

2434235

Fix typo

af84de8

zoecarver requested a review from slavapestov December 14, 2019 02:43

zoecarver added 2 commits December 14, 2019 10:11

Fix logic around checking dead instructions

7304d5a

Add comments and fix checking dead instructions

ea0698f

zoecarver commented Dec 14, 2019

View reviewed changes

slavapestov self-assigned this Dec 17, 2019

Merge branch 'master' into optimize/dead-global-vars

37fae30

zoecarver added 2 commits December 24, 2019 15:39

Fix should optimize logic

be6239a

Update requirements for test

1f66767

zoecarver commented Dec 24, 2019

View reviewed changes

slavapestov merged commit 3b9014d into swiftlang:master Jan 13, 2020

Optimize dead private global variables #28324

Optimize dead private global variables #28324

Uh oh!

Conversation

zoecarver commented Nov 18, 2019

Uh oh!

zoecarver commented Nov 18, 2019

Uh oh!

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

zoecarver commented Nov 20, 2019

Uh oh!

zoecarver commented Nov 24, 2019

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slavapestov Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

zoecarver Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

zoecarver Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slavapestov commented Dec 3, 2019

Uh oh!

zoecarver commented Dec 4, 2019

Uh oh!

zoecarver commented Dec 6, 2019

Uh oh!

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

Uh oh!

Uh oh!

zoecarver Dec 14, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Dec 17, 2019

Uh oh!

slavapestov commented Dec 17, 2019

Uh oh!

slavapestov commented Dec 17, 2019

Uh oh!

swift-ci commented Dec 17, 2019

Uh oh!

swift-ci commented Dec 17, 2019

Uh oh!

swift-ci commented Dec 17, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

zoecarver commented Dec 18, 2019

Uh oh!

theblixguy commented Dec 18, 2019

Uh oh!

eeckstein commented Dec 18, 2019

Uh oh!

theblixguy commented Dec 18, 2019

Uh oh!

eeckstein commented Dec 18, 2019

Uh oh!

zoecarver commented Dec 14, 2019 •

edited

Loading