[inst-simplify] Update for OSSA #34559

gottesmm · 2020-11-03T19:15:51Z

I am splitting this out and fixing some issues at atrick's request.

lib/SIL/IR/SILInstruction.cpp

atrick

Reviewed the first half, not including SimplifyInstruction or SILCombine

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

include/swift/SILOptimizer/Utils/ValueLifetime.h

lib/SIL/Utils/BasicBlockUtils.cpp

atrick

I don't think you should handle .none as an ownership kind with special treatment. Converting to .none can be trivially handled before doing anything fancy. Converting from .none involves deciding the ownership of each use, which should be a trivial lattice join between the new kind and the operand's constraint. Then partitioning those uses uses into two (or three) separate kinds, and for each category reusing the same code "as-if" the original .none value were either owned or guaranteed.

lib/SILOptimizer/Analysis/SimplifyInstruction.cpp

gottesmm · 2020-11-26T22:13:53Z

Going to rebase this.

…nstead of a SILInstructionVisitor. Our worklist already uses values, so it makes sense to use a SILValueVisitor. It will also let me throw in a few ARC dead argument elimination optimizations so clean up a pattern from my ARC RAUW thing. Once we have simplify-cfg it will not be needed, but I want to know I am good before I flip the switch.

gottesmm · 2020-12-09T00:19:55Z

I upstreamed and slimmed this down. I did it the way we talked about offline.

gottesmm · 2020-12-09T00:22:08Z

I think I also redid everything that were a problem.

gottesmm · 2020-12-09T00:23:59Z

@swift-ci test

gottesmm · 2020-12-09T00:24:04Z

@swift-ci benchmark

gottesmm · 2020-12-09T00:24:10Z

@swift-ci test source compatibility

gottesmm · 2020-12-09T00:24:35Z

preset=buildbot,tools=RA,stdlib=RD,test=non_executable
@swift-ci Please test with preset macOS

gottesmm · 2020-12-09T00:53:38Z

I am going to have to fight a little bit with MSVC looks like.

gottesmm · 2020-12-09T01:26:37Z

I am really curious what the benchmark results turn out. I diffed the text and there isn't any difference at -O anyways.

swift-ci · 2020-12-09T01:33:50Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
String.data.LargeUnicode	110	120	+9.1%	0.92x (?)
String.data.Medium	109	118	+8.3%	0.92x (?)

Code size: -O

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
DataAppendDataSmallToSmall	4140	3760	-9.2%	1.10x (?)

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: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 16 GB

gottesmm · 2020-12-09T01:54:15Z

If all of the tests pass, I am going to fix the windows issue and then smoke test this in.

atrick

Looks good overall. I'm still reviewing the RAUW utility and lifetime extender.

atrick · 2020-12-09T06:17:28Z

test/SILOptimizer/semantic-arc-opt-unchecked-ownership-conversion.sil

+sil [ossa] @owned_to_unowned_negative_1 : $@convention(thin) (@guaranteed Klass) -> () {
+bb0(%0 : @guaranteed $Klass):
+  %1 = copy_value %0 : $Klass
+  %2 = unchecked_ownership_conversion %1 : $Klass, @owned to @unowned
+  destroy_value %1 : $Klass


I just want to make sure I understand that the original test case is actually valid SIL. By default, I assume all our .sil tests are valid patterns that we need to support in perpetuity.

As I asked about in another comment, if this is valid SIL, then what keeps the optimizer from trashing the unowned value (e.g. after inlining)?

I would like to assume that it's the programmers responsibility to keep the unowned value alive via some sort of fixed lifetime, but I don't know if that's actually how the instruction is used in practice.

Nothing. Unowned means that you have to retain it before you use it as an owned or guaranteed value. Otherwise, you can just pass it off.

atrick · 2020-12-09T06:32:03Z

lib/SILOptimizer/SemanticARC/OwnershipConversionElimination.cpp

+bool SemanticARCOptVisitor::visitUncheckedOwnershipConversionInst(
+    UncheckedOwnershipConversionInst *uoci) {
+  // Return false if we are supposed to only be running guaranteed opts.
+  if (ctx.onlyGuaranteedOpts)


Just a thought: for a microsecond I was confused that onlyGuaranteedOpts referred to the ownership kind. Maybe the nomenclature should be MandatoryOpts (must run for correctness/diagnostics), AlwaysOnOpts (enabled even at -Onone for debug build performance), and PerformanceOpts (-O)

Why don't we talk about this offline. I'll do a follow up patch to rename to whatever makes sense.

lib/SILOptimizer/SemanticARC/OwnershipConversionElimination.cpp

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

include/swift/SILOptimizer/Utils/OwnershipUtils.h

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

lib/SILOptimizer/Utils/OwnershipUtils.cpp

include/swift/SILOptimizer/Utils/OwnershipUtils.h

atrick · 2020-12-09T08:05:16Z

lib/SILOptimizer/Utils/OwnershipUtils.cpp

+//===----------------------------------------------------------------------===//
+
+static SILBasicBlock::iterator
+replaceAllUsesAndEraseInner(SingleValueInstruction *svi, SILValue newValue,


Can there be just one copy of replaceAllUsesAndEraseInner?

lib/SILOptimizer/Utils/OwnershipUtils.cpp

… owned/guaranteed -> unowned. Specifically, we check that all of the unowned's value uses are within the lifetime of the unchecked_ownership_conversion's operand. If so, we eliminate it.

…i and enable SimplifyInstruction on OSSA. This is a generic API that when ownership is enabled allows one to replace all uses of a value with a value with a differing ownership by transforming/lifetime extending as appropriate. This API supports all pairings of ownership /except/ replacing a value with OwnershipKind::None with a value without OwnershipKind::None. This is a more complex optimization that we do not support today. As a result, we include on our state struct a helper routine that callers can use to know if the two values that they want to process can be handled by the algorithm. My moticiation is to use this to to update InstSimplify and SILCombiner in a less bug prone way rather than just turn stuff off. Noting that this transformation inserts ownership instructions, I have made sure to test this API in two ways: 1. With Mandatory Combiner alone (to make sure it works period). 2. With Mandatory Combiner + Semantic ARC Opts to make sure that we can eliminate the extra ownership instructions it inserts. As one can see from the tests, the optimizer today is able to handle all of these transforms except one conditional case where I need to eliminate a dead phi arg. I have a separate branch that hits that today but I have exposed unsafe behavior in ClosureLifetimeFixup that I need to fix first before I can land that. I don't want that to stop this PR since I think the current low level ARC optimizer may be able to help me here since this is a simple transform it does all of the time.

gottesmm · 2020-12-09T19:54:18Z

@swift-ci smoke test

I talked with atrick offline and he is fine with me doing additional changes post-commit.

gottesmm · 2020-12-09T22:31:11Z

Done... sooo happy!

gottesmm requested review from atrick and meg-gupta November 3, 2020 19:15

meg-gupta reviewed Nov 3, 2020

View reviewed changes

lib/SIL/IR/SILInstruction.cpp Outdated Show resolved Hide resolved

gottesmm force-pushed the ossa-inst-simplify branch from 4864f57 to db188f3 Compare November 3, 2020 23:02

atrick reviewed Nov 4, 2020

View reviewed changes

atrick previously requested changes Nov 4, 2020

View reviewed changes

gottesmm force-pushed the ossa-inst-simplify branch 2 times, most recently from 12e298d to 1c03b0d Compare November 5, 2020 01:13

gottesmm force-pushed the ossa-inst-simplify branch from 1c03b0d to 802f93f Compare December 9, 2020 00:18

gottesmm requested a review from atrick December 9, 2020 00:19

gottesmm force-pushed the ossa-inst-simplify branch from 802f93f to ec86fee Compare December 9, 2020 00:21

atrick reviewed Dec 9, 2020

View reviewed changes

[semantic-arc] Eliminate unneeded unchecked_ownership_conversion from…

6bd4a8e

… owned/guaranteed -> unowned. Specifically, we check that all of the unowned's value uses are within the lifetime of the unchecked_ownership_conversion's operand. If so, we eliminate it.

gottesmm force-pushed the ossa-inst-simplify branch from ec86fee to e2c3d87 Compare December 9, 2020 19:51

gottesmm force-pushed the ossa-inst-simplify branch from e2c3d87 to 259d2bb Compare December 9, 2020 19:54

gottesmm merged commit 1ca5577 into swiftlang:main Dec 9, 2020

gottesmm deleted the ossa-inst-simplify branch December 9, 2020 22:31

[inst-simplify] Update for OSSA #34559

[inst-simplify] Update for OSSA #34559

Uh oh!

Conversation

gottesmm commented Nov 3, 2020

Uh oh!

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atrick left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gottesmm commented Nov 26, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

swift-ci commented Dec 9, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Dec 9, 2020 • 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!

gottesmm Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

atrick Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atrick Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gottesmm commented Dec 9, 2020

Uh oh!

atrick left a comment •

edited

Loading

atrick Dec 9, 2020 •

edited

Loading