[ome] Invoke simplifyInstruction after lowering ownership and use replaceAllSimplifiedUsesAndErase instead of a manual RAUW. #34813

gottesmm · 2020-11-18T22:32:54Z

I am doing two things here:

simplifyInstruction expects its result to only be passed to
replaceAllSimplifiedUsesAndErase. This was an oversite.
I am upstreaming support for InstSimplify in OSSA (to finish sil-combine
work). The main assumption that inst-simplify is going to have is that it is
looking at SIL in correct ownership ssa or non-ownership ssa. It is not going to
be designed to work with an intermediate world that is the world where before
this PR it was being invoked (specifically, we have marked the function as not
being in OSSA but haven't finished transforming it into non-OSSA SIL). To avoid
this problem, I moved the simplification to the end of the pass after we have
done the initial traversal.

Should be NFCI.

gottesmm · 2020-11-18T22:33:02Z

gottesmm · 2020-11-18T23:26:56Z

Found the problem. It came down to emitDestroyValueOperation having a footman in it. I may try to fix it when I have time.

atrick · 2020-11-19T06:35:33Z

Not a big deal, but it's always helpful when renaming is done in a separate commit.

atrick

The change in this PR looks good. I asked for some clarification. The only issue I have is how badly SILBuilder is being abused for no reason. Why is the visitor passed a builder at all? The visitor should own a SILBuilderContext. It's trackingList should be that context's InsertedInsts. It can also hold on to the current builder, which can be instantiated in beforeVisit with the visitor's context.

Basically, don't call any setter functions on SILBuilder!

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp

gottesmm · 2020-11-19T22:37:05Z

@swift-ci smoke test

gottesmm · 2020-11-21T23:33:01Z

I am going to rebase this PR on top of #34844. #34844 fixes the bug that this PR was hitting (as one can see from the failing tests).

gottesmm · 2020-11-22T00:31:54Z

@swift-ci test

gottesmm · 2020-11-22T00:32:07Z

@swift-ci benchmark

swift-ci · 2020-11-22T00:47:40Z

Build failed
Swift Test Linux Platform
Git Sha - 6ad689c85a2ca8544f2830408bce27cdf6b657d4

swift-ci · 2020-11-22T01:10:21Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	1629	2501	+53.5%	0.65x (?)
FlattenListFlatMap	3935	4867	+23.7%	0.81x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced	4560	5020	+10.1%	0.91x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	1630	2510	+54.0%	0.65x (?)
FlattenListFlatMap	4233	5040	+19.1%	0.84x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
NSStringConversion.LongUTF8	3746	6282	+67.7%	0.60x
NSStringConversion.Long	13711	21515	+56.9%	0.64x
NSStringConversion.Medium	5236	7990	+52.6%	0.66x
NSStringConversion.UTF8	4816	7342	+52.5%	0.66x
NSStringConversion.MutableCopy.LongUTF8	2311	3516	+52.1%	0.66x
NSStringConversion.MutableCopy.Long	4223	6023	+42.6%	0.70x
NSStringConversion.MutableCopy.Medium	3496	4940	+41.3%	0.71x
NSStringConversion.MutableCopy.UTF8	2064	2641	+28.0%	0.78x (?)
NSStringConversion.Mutable	6004	7030	+17.1%	0.85x (?)
DataAppendBytesMedium	6240	6720	+7.7%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
Integrate	4348	4029	-7.3%	1.08x (?)

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

gottesmm · 2020-11-22T01:21:15Z

@swift-ci test linux platform

gottesmm · 2020-11-22T01:41:32Z

@swift-ci test

gottesmm · 2020-11-22T05:19:31Z

Woot all tests!

atrick · 2020-11-22T18:05:19Z

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp

+  withBuilder(SILInstruction *insertPt,
+              llvm::function_ref<ResultTy(SILBuilder &, SILLocation)> visitor) {
+    SILBuilderWithScope builder(insertPt, builderCtx);
+    return visitor(builder, insertPt->getLoc());


It's too bad this is so awkward. Ideally, building a single instruction should be able to naturally use function chaining:

SILBuilder(inst, builderCtx).emitSomethingElse(operands)

Yes. But today it is SILBuilderWithScope.

atrick

Thanks for fixing SILBuilder

gottesmm · 2020-11-26T22:05:44Z

Going to rebase again.

gottesmm · 2020-11-26T22:06:09Z

Going to rebase on top of #34877

gottesmm · 2020-11-26T22:07:14Z

@swift-ci test

…laceAllSimplifiedUsesAndErase instead of a manual RAUW. I am doing two things here: 1. simplifyInstruction expects its result to only be passed to replaceAllSimplifiedUsesAndErase. This was an oversite. 2. I am upstreaming support for InstSimplify in OSSA (to finish sil-combine work). The main assumption that inst-simplify is going to have is that it is looking at SIL in correct ownership ssa or non-ownership ssa. It is not going to be designed to work with an intermediate world that is the world where before this PR it was being invoked (specifically, we have marked the function as not being in OSSA but haven't finished transforming it into non-OSSA SIL). To avoid this problem, I moved the simplification to the end of the pass after we have done the initial traversal. Should be NFCI.

…SILBuilderWithContext and multiple local SILBuilderWithScope. To make it a little less verbose, I added an always inline helper method called: template <typename ResultTy> ResultTy OME::withBuilder(SILInstruction *insertPt, function_ref<ResultTy (SILBuilder &, SILLocation)> visitor) { SILBuilderWithScope builder(insertPt, builderContext); return visitor(builder, insertPt->getLoc()); } Since it is always inline there shouldn't be any perf impact and it should be like invoking a lambda in the caller directly in the same function that the lambda was declared in. I just couldn't type SILBuilderWithScope builder(insertPt, builderContext) all the time. Seems error prone.

gottesmm · 2020-11-30T19:23:55Z

Rebased on top of main now that I landed the precursors.

gottesmm · 2020-11-30T19:24:03Z

@swift-ci test

gottesmm requested a review from atrick November 18, 2020 22:33

atrick approved these changes Nov 19, 2020

View reviewed changes

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp Outdated Show resolved Hide resolved

gottesmm force-pushed the pr-363155fab264654b07657a22133eafbe01f5d6d1 branch 3 times, most recently from 0f315b4 to 9284782 Compare November 19, 2020 22:16

gottesmm force-pushed the pr-363155fab264654b07657a22133eafbe01f5d6d1 branch 2 times, most recently from 71d1f14 to 6ad689c Compare November 22, 2020 00:31

gottesmm force-pushed the pr-363155fab264654b07657a22133eafbe01f5d6d1 branch from 6ad689c to 4ba0496 Compare November 22, 2020 01:41

atrick reviewed Nov 22, 2020

View reviewed changes

atrick approved these changes Nov 22, 2020

View reviewed changes

gottesmm force-pushed the pr-363155fab264654b07657a22133eafbe01f5d6d1 branch from 4ba0496 to 679a250 Compare November 26, 2020 22:07

gottesmm added 2 commits November 30, 2020 11:23

gottesmm force-pushed the pr-363155fab264654b07657a22133eafbe01f5d6d1 branch from 679a250 to f9e0649 Compare November 30, 2020 19:23

gottesmm merged commit c467bf9 into swiftlang:main Nov 30, 2020

gottesmm deleted the pr-363155fab264654b07657a22133eafbe01f5d6d1 branch November 30, 2020 23:31

[ome] Invoke simplifyInstruction after lowering ownership and use replaceAllSimplifiedUsesAndErase instead of a manual RAUW. #34813

[ome] Invoke simplifyInstruction after lowering ownership and use replaceAllSimplifiedUsesAndErase instead of a manual RAUW. #34813

Uh oh!

Conversation

gottesmm commented Nov 18, 2020

Uh oh!

gottesmm commented Nov 18, 2020

Uh oh!

gottesmm commented Nov 18, 2020

Uh oh!

atrick commented Nov 19, 2020

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gottesmm commented Nov 19, 2020

Uh oh!

gottesmm commented Nov 21, 2020

Uh oh!

gottesmm commented Nov 22, 2020

Uh oh!

gottesmm commented Nov 22, 2020

Uh oh!

swift-ci commented Nov 22, 2020

Uh oh!

swift-ci commented Nov 22, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

gottesmm commented Nov 22, 2020

Uh oh!

gottesmm commented Nov 22, 2020

Uh oh!

gottesmm commented Nov 22, 2020

Uh oh!

atrick Nov 22, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Nov 26, 2020

Uh oh!

gottesmm commented Nov 26, 2020

Uh oh!

gottesmm commented Nov 26, 2020

Uh oh!

gottesmm commented Nov 30, 2020

Uh oh!

gottesmm commented Nov 30, 2020

Uh oh!

Uh oh!