Enable CSE on OSSA #34895

meg-gupta · 2020-11-30T21:42:17Z

No description provided.

lib/SILOptimizer/Transforms/CSE.cpp

gottesmm · 2020-12-01T01:42:07Z

I started perusing maybe I missed, do you have a bunch of tests with guaranteed values?

meg-gupta · 2020-12-01T09:21:59Z

@gottesmm I added some tests with guaranteed values now.

meg-gupta · 2020-12-01T21:05:40Z

@swift-ci test

swift-ci · 2020-12-01T22:22:30Z

Build failed
Swift Test Linux Platform
Git Sha - c035ffba046d5a1f17952503dad48155e2639a44

swift-ci · 2020-12-01T22:54:11Z

Build failed
Swift Test OS X Platform
Git Sha - c035ffba046d5a1f17952503dad48155e2639a44

include/swift/SIL/SILInstruction.h

lib/SILOptimizer/Transforms/CSE.cpp

zoecarver · 2020-12-03T17:49:29Z

lib/SILOptimizer/Transforms/CSE.cpp

+        // new opened archetype already registered in
+        // CSE::processOpenExistentialRefInst.
+        auto *oldOpenedArchetypeDef = oldUse->getUser();
+        auto &Builder = Cloner.getBuilder();


nit: Builder -> builder.

lib/SILOptimizer/Transforms/CSE.cpp

zoecarver · 2020-12-03T18:19:01Z

lib/SILOptimizer/Transforms/CSE.cpp

-        RunsOnHighLevelSil(RunsOnHighLevelSil) {}
+        OpenedArchetypesTracker(OpenedArchetypesTracker), Cloner(Cloner),
+        RunsOnHighLevelSil(RunsOnHighLevelSil) {
+    auto &Builder = Cloner.getBuilder();


Nit: also no captial. Or maybe even: Cloner.getBuilder().setOpenedArchetypesTracker...

lib/SILOptimizer/Transforms/CSE.cpp

meg-gupta · 2020-12-03T23:38:10Z

@swift-ci test

swift-ci · 2020-12-04T01:12:05Z

Build failed
Swift Test OS X Platform
Git Sha - 85cfe4388469cf726c3427b92132a2dc2d7b0a19

meg-gupta · 2020-12-15T07:53:07Z

@swift-ci test

meg-gupta · 2020-12-16T02:49:30Z

@swift-ci test

swift-ci · 2020-12-16T03:23:45Z

Build failed
Swift Test OS X Platform
Git Sha - 15c9dd1f1c89fa1a1c2dc62a78ad7241455b0d53

meg-gupta · 2020-12-16T07:24:53Z

@swift-ci test

zoecarver

This lgtm now. Thanks for all your work on this :)

zoecarver · 2020-12-16T18:47:55Z

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

-    auto *innerBorrow =
-        reborrowBuilder.createBeginBorrow(bi->getLoc(), innerCopy);
-    auto *outerEndBorrow = reborrowBuilder.createEndBorrow(bi->getLoc(), value);
+    SILLocation loc = RegularLocation(bi->getLoc().getSourceLoc());


Nit: couldn't this just be bi->getLoc()?

zoecarver · 2020-12-16T18:48:57Z

lib/SILOptimizer/Transforms/CSE.cpp

@@ -1284,22 +1372,32 @@ class SILCSE : public SILFunctionTransform {
    auto *SEA = PM->getAnalysis<SideEffectAnalysis>();
    SILOptFunctionBuilder FuncBuilder(*this);

-    CSE C(RunsOnHighLevelSil, SEA, FuncBuilder);
+    auto *func = getFunction();
+    SILOpenedArchetypesTracker OpenedArchetypesTracker(func);


Nit: camelCase not TitleCase.

gottesmm

Some additional comments. I need to read this locally before giving it my +1. I am doing that now.

gottesmm · 2020-12-17T20:51:09Z

lib/SILOptimizer/Transforms/CSE.cpp

+    auto Operands = X->getOperandValues();
+    SmallVector<SILValue, 4> OperandValues;
+    llvm::transform(
+        Operands, std::back_inserter(OperandValues),


One thought. Is it possible to create a range that does this transform and pass that to hash_combine instead? That way we do not intermediate arrays (and potential mallocs).

(Keep in mind I am just tagging this one, but I am talking about all of the places that this was done.

I'll try to do this

I added a transform function as an optional argument to SILInstruction::getOperandValues. It may be useful in other code as well.

gottesmm · 2020-12-17T20:53:18Z

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -73,6 +73,10 @@ Optional<SILBasicBlock::iterator> swift::getInsertAfterPoint(SILValue val) {
  if (isa<SingleValueInstruction>(val)) {


why not:

if (auto *inst = val->getDefiningInstruction()) { return std::next(inst->getIterator()); }

gottesmm · 2020-12-17T21:04:34Z

@meg-gupta I was talking with Andy about this issue around ValueLifetimeAnalysis and returns. I think we are hacking around bugs here. We need a way to have a /real/ location given a terminator insert pt and value lifetime analysis should be smart enough about return inst.

gottesmm · 2020-12-20T23:07:25Z

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

-  bool result = lifetimeAnalysis.computeFrontier(
-      frontier, ValueLifetimeAnalysis::DontModifyCFG, &ctx.deBlocks);
-  assert(result);
+  bool noCriticalEdges = lifetimeAnalysis.computeFrontier(


I think this is the wrong way to do this. Why is this needed?

Specifically, if it is needed due to the terminator issue we spoke about before, I think that Andy's recent change adding UnownedInstantaneousUse makes that not necessary for unowned things

You are right, this was not needed. It turned out to be a bug where the ownership rauw was considering transitive uses of none ownership as well.

gottesmm

More initial comments.

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

include/swift/SIL/SILInstruction.h

include/swift/SIL/InstructionUtils.h

lib/SILOptimizer/Transforms/CSE.cpp

…value/destroy_value etc Branch instructions and frontiers can have LocationKind::ReturnKind/ImplicitReturnKind which are not correct locations to use for copy_value/destroy_value etc

gottesmm

Some more final comments! Looks pretty good! I think we are close!

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

lib/SILOptimizer/Utils/InstOptUtils.cpp

include/swift/SIL/SILInstruction.h

lib/SILOptimizer/Transforms/CSE.cpp

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

lib/SILOptimizer/Transforms/CSE.cpp

meg-gupta · 2020-12-23T01:25:39Z

@swift-ci test

meg-gupta · 2020-12-23T01:25:48Z

@swift-ci benchmark

swift-ci · 2020-12-23T02:41:44Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
Data.hash.Medium	25	28	+12.0%	0.89x (?)

Improvement	OLD	NEW	DELTA	RATIO
LessSubstringSubstring	29	22	-24.1%	1.32x
EqualSubstringSubstring	29	22	-24.1%	1.32x
EqualSubstringString	29	22	-24.1%	1.32x (?)
LessSubstringSubstringGenericComparable	29	22	-24.1%	1.32x
EqualStringSubstring	30	23	-23.3%	1.30x (?)
EqualSubstringSubstringGenericEquatable	30	23	-23.3%	1.30x
StringComparison_longSharedPrefix	358	321	-10.3%	1.12x (?)
SortStringsUnicode	2275	2080	-8.6%	1.09x (?)
UTF8Decode_InitDecoding_ascii	203	189	-6.9%	1.07x (?)
Data.init.Sequence.513B.Count0.I	322	300	-6.8%	1.07x (?)
AngryPhonebook.Armenian	136	127	-6.6%	1.07x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
StringHasPrefixAscii	980	1060	+8.2%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
LessSubstringSubstring	29	22	-24.1%	1.32x
EqualSubstringSubstring	29	22	-24.1%	1.32x (?)
EqualStringSubstring	29	22	-24.1%	1.32x
EqualSubstringSubstringGenericEquatable	29	22	-24.1%	1.32x
EqualSubstringString	29	22	-24.1%	1.32x
LessSubstringSubstringGenericComparable	29	22	-24.1%	1.32x
AngryPhonebook.Cyrillic	148	138	-6.8%	1.07x (?)
AngryPhonebook.Armenian	136	127	-6.6%	1.07x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
DropLastCountableRange	182	201	+10.4%	0.91x (?)
ConvertFloatingPoint.MockFloat64ToDouble	1052	1159	+10.2%	0.91x (?)
ObjectiveCBridgeStubToNSStringRef	128	141	+10.2%	0.91x (?)
PrefixCountableRange	526	573	+8.9%	0.92x (?)
Data.hash.Medium	36	39	+8.3%	0.92x (?)
ObjectiveCBridgeStubToNSDateRef	2900	3140	+8.3%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
LessSubstringSubstring	89	81	-9.0%	1.10x
NSStringConversion.MutableCopy.LongUTF8	933	851	-8.8%	1.10x (?)
LessSubstringSubstringGenericComparable	86	79	-8.1%	1.09x (?)
EqualSubstringSubstring	88	81	-8.0%	1.09x (?)
NSStringConversion.MutableCopy.Long	1335	1237	-7.3%	1.08x (?)
AngryPhonebook.Armenian	137	127	-7.3%	1.08x (?)
AngryPhonebook.Cyrillic	148	138	-6.8%	1.07x (?)

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: 6-Core 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

transformed values of the operands

@guaranteed

…auw of guaranteed values When we are populating transitive users while handling guaranteed values in ownership rauw, we were including values with none ownership. Example : rauw of %2 with a dominating value Here %arg1 was also considered a transitive use %2 = struct_extract %0 : $StructWithEnum2, #StructWithEnum2.val %copy = copy_value %2 : $FakeOptional2 switch_enum %2 : $FakeOptional2, case #FakeOptional2.some1!enumelt:bb5, case #FakeOptional2.some2!enumelt:bb6 bb5(%arg1 : $UInt): br bb7(%arg1 : $UInt) bb6(%arg2 : @guaranteed $Klass): %4 = unchecked_trivial_bit_cast %arg2 : $Klass to $UInt br bb7(%4 : $UInt) This is incorrect because %arg1 is a trivial value, and this also leads to ValueLifetimeAnalysis needing a split for finding a frontier for the use of %arg1 in the branch instruction. In ossa, we should never have to split edges for finding frontiers, because we do not have critical edges.

meg-gupta · 2020-12-23T06:23:00Z

@swift-ci test

meg-gupta · 2020-12-23T07:27:03Z

@swift-ci test

meg-gupta · 2020-12-23T17:58:37Z

@gottesmm I am merging this. I'll add a follow on commit if there are any other comments.

zoecarver · 2020-12-23T18:47:08Z

Woohoo! So excited to see ownership making its way through the SILOptimizer. Thanks for all your work on this, @meg-gupta.

meg-gupta requested review from gottesmm and atrick November 30, 2020 21:42

meg-gupta force-pushed the cseossa branch from 428d9d4 to 740226d Compare November 30, 2020 22:49

gottesmm reviewed Dec 1, 2020

View reviewed changes

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

meg-gupta force-pushed the cseossa branch 2 times, most recently from 8c462b5 to e9a610b Compare December 1, 2020 09:21

meg-gupta force-pushed the cseossa branch from e9a610b to 25eb78b Compare December 1, 2020 19:18

meg-gupta force-pushed the cseossa branch from c035ffb to 4e0fc98 Compare December 2, 2020 22:09

zoecarver reviewed Dec 3, 2020

View reviewed changes

meg-gupta force-pushed the cseossa branch from 4e0fc98 to 5317a1c Compare December 3, 2020 20:49

meg-gupta commented Dec 3, 2020

View reviewed changes

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

zoecarver reviewed Dec 3, 2020

View reviewed changes

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

meg-gupta force-pushed the cseossa branch from 5317a1c to dfcf66d Compare December 3, 2020 21:17

zoecarver reviewed Dec 3, 2020

View reviewed changes

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

meg-gupta force-pushed the cseossa branch 2 times, most recently from 9cbf2cd to 85cfe43 Compare December 3, 2020 23:23

meg-gupta force-pushed the cseossa branch 4 times, most recently from b5be2e3 to e999492 Compare December 15, 2020 07:52

meg-gupta force-pushed the cseossa branch from e999492 to e73394b Compare December 15, 2020 07:55

meg-gupta force-pushed the cseossa branch from 15c9dd1 to d7dfcfb Compare December 16, 2020 07:24

zoecarver approved these changes Dec 16, 2020

View reviewed changes

gottesmm reviewed Dec 17, 2020

View reviewed changes

meg-gupta force-pushed the cseossa branch from d7dfcfb to b419961 Compare December 17, 2020 23:02

gottesmm reviewed Dec 20, 2020

View reviewed changes

meg-gupta force-pushed the cseossa branch 2 times, most recently from b97dbb1 to 138b4e8 Compare December 21, 2020 04:25

In OSSA rauw utilities use auto generated location for creating copy_…

c7fe393

…value/destroy_value etc Branch instructions and frontiers can have LocationKind::ReturnKind/ImplicitReturnKind which are not correct locations to use for copy_value/destroy_value etc

meg-gupta force-pushed the cseossa branch from 138b4e8 to 7e279b7 Compare December 21, 2020 23:04

gottesmm reviewed Dec 23, 2020

View reviewed changes

meg-gupta added 4 commits December 22, 2020 22:01

Add a new overload of SILInstruction::getOperandValues to get

372efd5

transformed values of the operands

Rename stripOwnershipInsts to lookThroughCopyValueInsts

68ec810

Support MultipleValueInstruction in swift::getInsertAfterPoint

8431ceb

meg-gupta force-pushed the cseossa branch from 7e279b7 to ba3afe2 Compare December 23, 2020 06:22

Enable CSE on OSSA

42c0319

meg-gupta force-pushed the cseossa branch from ba3afe2 to 42c0319 Compare December 23, 2020 07:26

meg-gupta merged commit b99533a into swiftlang:main Dec 23, 2020

		@@ -73,6 +73,10 @@ Optional<SILBasicBlock::iterator> swift::getInsertAfterPoint(SILValue val) {
		if (isa<SingleValueInstruction>(val)) {

Enable CSE on OSSA #34895

Enable CSE on OSSA #34895

Uh oh!

Conversation

meg-gupta commented Nov 30, 2020

Uh oh!

Uh oh!

gottesmm commented Dec 1, 2020

Uh oh!

meg-gupta commented Dec 1, 2020

Uh oh!

meg-gupta commented Dec 1, 2020

Uh oh!

swift-ci commented Dec 1, 2020

Uh oh!

swift-ci commented Dec 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meg-gupta commented Dec 3, 2020

Uh oh!

swift-ci commented Dec 4, 2020

Uh oh!

meg-gupta commented Dec 15, 2020

Uh oh!

meg-gupta commented Dec 16, 2020

Uh oh!

swift-ci commented Dec 16, 2020

Uh oh!

meg-gupta commented Dec 16, 2020

Uh oh!

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

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!

gottesmm commented Dec 17, 2020

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!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!