DeadObjectElimination: delete dead arrays for which the "destroyArray" builtin is inlined. #35737

eeckstein · 2021-02-03T18:32:58Z

This is needed for non-OSSA SIL: in case we see a destroyArray builtin, we can safely remove the otherwise dead array.
This optimization kicks in later in the pipeline, after array semantics are already inlined (for early SIL, DeadObjectElimination can remove arrays based on semantics).

rdar://73569282
https://bugs.swift.org/browse/SR-14100

eeckstein · 2021-02-03T18:33:08Z

@swift-ci test

eeckstein · 2021-02-03T18:33:16Z

@swift-ci benchmark

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

swift-ci · 2021-02-03T19:12:06Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	1653	2514	+52.1%	0.66x (?)
FlattenListFlatMap	4821	6695	+38.9%	0.72x (?)
NSStringConversion.MutableCopy.Rebridge	810	954	+17.8%	0.85x (?)
ObjectiveCBridgeFromNSSetAnyObjectToStringForced	88000	95500	+8.5%	0.92x (?)

Code size: -O

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	6666	6097	-8.5%	1.09x (?)

Code size: -Osize

Performance: -Onone

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 Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

atrick

Fantastic. Thanks!

fyi, the OSSA SILCombine can be tested now, although I realize this is all about late inlining.

atrick · 2021-02-03T19:27:37Z

lib/SILOptimizer/Transforms/DeadObjectElimination.cpp

+    // Check if the store is within the range of the destroyed array. In OSSA
+    // we would not need this check. Otherwise it would be a memory lifetime
+    // failure.
+    if (storePath.getOffset() < 0 || storePath.getOffset() >= numDestroyed)


Wow, I can't imagine how this check could be hit, but it doesn't hurt to check I guess.

Yeah, it's not something which is ever generated from swift source, but in SIL it's legal.

Needed to remove unneeded reference counting for String literals.

…" builtin is inlined. This is needed for non-OSSA SIL: in case we see a destroyArray builtin, we can safely remove the otherwise dead array. This optimization kicks in later in the pipeline, after array semantics are already inlined (for early SIL, DeadObjectElimination can remove arrays based on semantics). rdar://73569282 https://bugs.swift.org/browse/SR-14100

The offset was not compared.

eeckstein · 2021-02-04T06:54:00Z

@swift-ci smoke test and merge

drodriguez · 2021-02-05T01:25:36Z

Hi,

I was looking at why the Android CI has been failing since https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-16.04-android/9117/ and it seems to be because of the changes introduced in the test in this commit. No need to fix it or anything, I am just trying to understand the changes, and how it might be have affected only Android.

This is the failure.

******************** TEST 'Swift(android-armv7) :: SILOptimizer/dead_array_elim.swift' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/buildbot_linux/swift-linux-x86_64/bin/swift-frontend -target armv7-unknown-linux-android -I /home/ubuntu/android-ndk-r19c/sysroot/usr/include -I /home/ubuntu/android-ndk-r19c/sysroot/usr/include/arm-linux-androideabi -L /home/ubuntu/android-ndk-r19c/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a -L /home/ubuntu/android-ndk-r19c/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/lib/gcc/arm-linux-androideabi/4.9.x -L /home/ubuntu/android-ndk-r19c/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/arm-linux-androideabi/lib  -module-cache-path /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/buildbot_linux/swift-linux-x86_64/swift-test-results/armv7-unknown-linux-androideabi/clang-module-cache -swift-version 4  -ignore-module-source-info -typo-correction-limit 10  -O -emit-sil -primary-file /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/swift/test/SILOptimizer/dead_array_elim.swift | /usr/bin/python3.5 /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/swift/utils/PathSanitizingFileCheck --sanitize BUILD_DIR=/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/buildbot_linux/swift-linux-x86_64 --sanitize SOURCE_DIR=/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/swift --use-filecheck /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/buildbot_linux/llvm-linux-x86_64/bin/FileCheck  /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/swift/test/SILOptimizer/dead_array_elim.swift
--
Exit Code: 1

Command Output (stderr):
--
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/swift/test/SILOptimizer/dead_array_elim.swift:58:16: error: CHECK-NEXT: is not on the line after the previous match
// CHECK-NEXT: integer_literal $Builtin.Int{{[0-9]+}}, 21
               ^
<stdin>:99:8: note: 'next' match was here
 %18 = integer_literal $Builtin.Int32, 21 // user: %19
       ^
<stdin>:82:55: note: previous match ended here
 debug_value %0 : $String, let, name "stringParameter", argno 1 // id: %1
                                                      ^
<stdin>:83:1: note: non-matching line after previous match is here
 %2 = integer_literal $Builtin.Int32, 1819043176 // user: %3
^

Input file: <stdin>
Check file: /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android/swift/test/SILOptimizer/dead_array_elim.swift

-dump-input=help explains the following input dump.

Input was:
<<<<<<
         .
         .
         .
        94:  %13 = struct $String (%12 : $_StringGuts) // users: %16, %15
        95:  retain_value %0 : $String // id: %14
        96:  release_value %13 : $String // id: %15
        97:  release_value %13 : $String // id: %16
        98:  release_value %0 : $String // id: %17
        99:  %18 = integer_literal $Builtin.Int32, 21 // user: %19
next:58            !~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~              error: match on wrong line
       100:  %19 = struct $Int (%18 : $Builtin.Int32) // user: %20
       101:  return %19 : $Int // id: %20
       102: } // end sil function '$s15dead_array_elim31testDeadArrayAfterOptimizationsySiSSF'
       103: 
       104: // static Array._adoptStorage(_:count:)
         .
         .
         .
>>>>>>

--

********************

From my understanding of the test (and the JIRA ticket), it seems that the final value 21 is indeed created, but there's seems to be something in between constructing the return value and returning it, and the debug_value line. Those lines are retain_value/release_value, which seems like something is being constructed and destroyed (maybe what these changes tried to remove).

This is Android 32 bits (64 bits works fine), and if I understand correctly Darwin doesn't have a lot of 32 bits platforms lying around anymore, but https://github.com/apple/swift/pull/35737/files#diff-c34c7dab63c3329cf8671662271a8d64c1ee918eff414b91196fe5df89883265R259 looks like something that might not be true for a 32 bit platform.

Any insight will be appreciated. If nobody have any idea, I can simply disable the test with an XFAIL.

eeckstein · 2021-02-05T07:56:25Z

@drodriguez Thanks for letting me know!
I fixed it in #35776

eeckstein requested a review from atrick February 3, 2021 18:33

atrick reviewed Feb 3, 2021

View reviewed changes

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp Show resolved Hide resolved

atrick approved these changes Feb 3, 2021

View reviewed changes

eeckstein added 4 commits February 4, 2021 07:53

SIL: add two utility functions to AccessPath

f12ac4c

SILCombine: remove release_value of a value_to_bridge_object

34ae1d9

Needed to remove unneeded reference counting for String literals.

SIL: fix the == operator for AccessPath

1655e22

The offset was not compared.

eeckstein force-pushed the doe-destroy-array branch from 2d3b20a to 1655e22 Compare February 4, 2021 06:53

swift-ci merged commit 7a7237b into swiftlang:main Feb 4, 2021

eeckstein deleted the doe-destroy-array branch February 4, 2021 11:17

eeckstein mentioned this pull request Feb 5, 2021

SILCombine: remove release_value of a trivial enum #35776

Merged

drodriguez mentioned this pull request Feb 26, 2021

[ownership] Move ownership lowering after serialization on stdlibCore. #35872

Merged

eeckstein mentioned this pull request Feb 8, 2021

[SR-14100] Swift doesn't quite understand that optimising out whole Arrays is fine #56486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DeadObjectElimination: delete dead arrays for which the "destroyArray" builtin is inlined. #35737

DeadObjectElimination: delete dead arrays for which the "destroyArray" builtin is inlined. #35737

Uh oh!

eeckstein commented Feb 3, 2021

Uh oh!

eeckstein commented Feb 3, 2021

Uh oh!

eeckstein commented Feb 3, 2021

Uh oh!

Uh oh!

swift-ci commented Feb 3, 2021

Uh oh!

atrick left a comment

Uh oh!

atrick Feb 3, 2021

Uh oh!

eeckstein Feb 4, 2021

Uh oh!

eeckstein commented Feb 4, 2021

Uh oh!

drodriguez commented Feb 5, 2021

Uh oh!

eeckstein commented Feb 5, 2021

Uh oh!

Uh oh!

DeadObjectElimination: delete dead arrays for which the "destroyArray" builtin is inlined. #35737

DeadObjectElimination: delete dead arrays for which the "destroyArray" builtin is inlined. #35737

Uh oh!

Conversation

eeckstein commented Feb 3, 2021

Uh oh!

eeckstein commented Feb 3, 2021

Uh oh!

eeckstein commented Feb 3, 2021

Uh oh!

Uh oh!

swift-ci commented Feb 3, 2021

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

eeckstein Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

eeckstein commented Feb 4, 2021

Uh oh!

drodriguez commented Feb 5, 2021

Uh oh!

eeckstein commented Feb 5, 2021

Uh oh!

Uh oh!