SILGen: simplify cleanups and avoid critical edges. #19886

atrick · 2018-10-15T18:18:48Z

Keep the CFG in a valid, critical-edge free state that the SIL passes can handle.
Replace the ad-hoc on-the-fly cleanup block optimization with a more general block merging.

This simplifies SILGen now and will in subsequent commits allow massive simplification of SIL APIs and SIL passes.

Note that there's no downside to avoiding critical edges since they will be split anyway any various points in the SIL pipeline. However, there are many passes and utilities that can be made much simpler and more compute time and memory efficient.

atrick · 2018-10-15T18:26:19Z

@swift-ci test.

atrick · 2018-10-15T18:27:05Z

@swift-ci benchmark.

atrick · 2018-10-15T18:27:26Z

@swift-ci test source compatibility.

swift-ci · 2018-10-15T19:13:37Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1617	1782	+10.2%	0.91x
Improvement
StringHashing_abnormal	1461	1350	-7.6%	1.08x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1580	1807	+14.4%	0.87x
RemoveWhereFilterString	254	278	+9.4%	0.91x
CStringLongAscii	3278	3532	+7.7%	0.93x
Improvement
StringHashing_abnormal	1479	1359	-8.1%	1.09x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
ArrayOfPOD	781	859	+10.0%	0.91x (?)

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

swift-ci · 2018-10-15T19:20:22Z

Build failed
Swift Test Linux Platform
Git Sha - 4b8d211c4a911e1cc0b39a8fb2b78d67ff2d7691

swift-ci · 2018-10-15T19:45:44Z

Build failed
Swift Test OS X Platform
Git Sha - 4b8d211c4a911e1cc0b39a8fb2b78d67ff2d7691

gottesmm

Looks like you are dicing/splicing. I think I found some nice feedback. Want to give it to you before you slice and dice more.

lib/SILGen/Condition.h

lib/SILGen/Condition.cpp

lib/SILGen/SILGenFunction.h

gottesmm · 2018-10-15T22:56:27Z

lib/SILGen/Condition.cpp

-
-  SGF.B.emitBlock(TrueBB);
+void Condition::enter(SILGenFunction &SGF, SILBasicBlock *destBB) {
+  assert(destBB && "Cannot enter a finished branch.");


branch => block?

lib/SILGen/Condition.h

lib/SILGen/SILGenExpr.cpp

gottesmm

One last one.

lib/SILGen/SILGenFunction.h

atrick · 2018-10-16T00:31:04Z

@gottesmm thanks for looking at this. I'd like to add feedback from review as separate commits so you don't need to re-review anything, but in this case, you hadn't finished the first round, and rebasing was part of the feedback. So the commits are all new, but nothing has changed functionally. Unfortunately, github isn't listing the commits in the right order anymore--trust that they are in the right order on the branch. Incidentally, I think it's better to clone-for-review for anything non-trivial.

gottesmm

I still think 66bc68151dcfcc9c45869f08fab4220054adf12e should be before the previous one. What you would do is do the same change but make it so that you can pass false in that place. Then all usages of Condition pass false and we can eliminate that unfortunate code... no? I think together the end state is fine though.

Also:

I wanted to give a +1 on a particular comment. It reads awesome!
Found a typo.

Beyond that LGTM.

gottesmm · 2018-10-16T00:57:35Z

lib/SILGen/Condition.h

@@ -32,10 +32,12 @@ namespace Lowering {

 /// A condition is the result of evaluating a boolean expression as
 /// control flow.
+///
+/// For each Condition instance, `enterTrue` must be called before `complete`.
+/// If `enterFalse` is skipped, then an empty fall-through block is created.


Nice comment!

gottesmm · 2018-10-16T00:57:57Z

lib/SILGen/Condition.h

  /// complete - Complete this conditional execution.  This should be called
  /// only after all other calls on this Condition have been made.
+  /// This leaves SGF's SILGenBuilder at the continution block.


continution => continuation

lib/SILGen/SILGenFunction.h

lib/SILGen/SILGenExpr.cpp

atrick · 2018-10-16T06:26:29Z

@swift-ci benchmark.

atrick · 2018-10-16T06:51:31Z

Rerunning benchmarks to see if the few changes were spurious. Locally, I see only improvements. In particular, CI reported a 10% regression on IterateData, but I see a 10% improvement (although it seems very flaky). The only reason for regression would be incidental change to inlining because the cost model doesn't handle unconditional branches sensibly.

For code size, there is a 0.1% decrease in the benchmark suite and 0.1% increase in the stdlib.

atrick · 2018-10-16T06:51:40Z

@swift-ci benchmark.

atrick · 2018-10-16T06:52:30Z

I see no measurable impact on compile time.

swift-ci · 2018-10-16T07:37:00Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Improvement
CStringLongAscii	3533	3290	-6.9%	1.07x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1588	1770	+11.5%	0.90x (?)
Improvement
CStringLongAscii	3528	3276	-7.1%	1.08x

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 · 2018-10-19T22:49:58Z

@swift-ci test.

atrick · 2018-10-19T22:50:48Z

@swift-ci test.

swift-ci · 2018-10-20T00:11:15Z

Build failed
Swift Test OS X Platform
Git Sha - e889f15b3f1068c074de88862a71ef16810959ee

This avoids critical edges, which will be split in the optimizer anyway.

Allow a new SILBuilder to be created for an insertion point, while providing all of its necessary context. Ultimately, the builder's constructor should take an insertion point, DebugLocation, and context. Then we won't need to pass SILLocation to all of its methods. This makes much more sense and is much safer than saving the insertion point via an RAII object or defining a separate SILBuilderWithScope. Those broken abstractions should go away.

This is the best way to inherit the provided SILGenBuilder's debug scope and context, but insert at a different SIL location.

This code was attempting to generate a critical edge on one side of a condition. Not only is that undesirable, but it breaks the abstraction and results an a lot of complexity. As usual, fixing critical edges simply means removing unnecessary complexity.

This is the one place in SILGen where we need to explicitly split the critical edge. createBasicBlockAndBranch is a simply utility for that.

Avoid emitting unnecessary basic block for cleanup chains. This is a general approach that handles all cases while simplifying SILGen emission and keeping the CFG in a valid state during SILGen.

atrick · 2018-10-20T06:15:03Z

@swift-ci test.

atrick · 2018-10-20T06:16:17Z

@swift-ci test.

atrick requested a review from gottesmm October 15, 2018 18:26

gottesmm reviewed Oct 15, 2018

View reviewed changes

lib/SILGen/SILGenFunction.h Show resolved Hide resolved

gottesmm reviewed Oct 16, 2018

View reviewed changes

atrick mentioned this pull request Oct 16, 2018

Canonicalize Definit-init conditional branch creation. #19913

Merged

atrick added 9 commits October 19, 2018 22:22

SILGen: Fix a variable name conflict.

2e71400

SILGen: Always emit the cleanup block.

e04378f

This avoids critical edges, which will be split in the optimizer anyway.

Add a SILGenBuilder constructor that takes a new insertion point.

f73e6c7

This is the best way to inherit the provided SILGenBuilder's debug scope and context, but insert at a different SIL location.

Simplify SILGen/Condition.

1c701b2

This code was attempting to generate a critical edge on one side of a condition. Not only is that undesirable, but it breaks the abstraction and results an a lot of complexity. As usual, fixing critical edges simply means removing unnecessary complexity.

SILGen KeyPath: Create a basic block to split the critical edge.

c8e4d2b

This is the one place in SILGen where we need to explicitly split the critical edge. createBasicBlockAndBranch is a simply utility for that.

SILGenFunction::mergeCleanupBlocks.

2ccc089

Avoid emitting unnecessary basic block for cleanup chains. This is a general approach that handles all cases while simplifying SILGen emission and keeping the CFG in a valid state during SILGen.

Fix test cases for SILGen after removing critical edges.

ba03bef

Add FIXME comments with suggestions to improve test case maintenance.

a03d7bf

atrick merged commit c3fed98 into swiftlang:master Oct 20, 2018

atrick deleted the silgen-critedge branch September 3, 2019 01:57

SILGen: simplify cleanups and avoid critical edges. #19886

SILGen: simplify cleanups and avoid critical edges. #19886

Uh oh!

Conversation

atrick commented Oct 15, 2018

Uh oh!

atrick commented Oct 15, 2018

Uh oh!

atrick commented Oct 15, 2018

Uh oh!

atrick commented Oct 15, 2018

Uh oh!

swift-ci commented Oct 15, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

swift-ci commented Oct 15, 2018

Uh oh!

swift-ci commented Oct 15, 2018

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gottesmm Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

atrick Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atrick commented Oct 16, 2018

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

gottesmm Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atrick commented Oct 16, 2018

Uh oh!

atrick commented Oct 16, 2018

Uh oh!

atrick commented Oct 16, 2018

Uh oh!

atrick commented Oct 16, 2018

Uh oh!

swift-ci commented Oct 16, 2018

Build comment file:

Performance: -O

Performance: -Osize

Uh oh!

atrick commented Oct 19, 2018

Uh oh!

atrick commented Oct 19, 2018

Uh oh!

swift-ci commented Oct 20, 2018

Uh oh!

atrick commented Oct 20, 2018

Uh oh!

atrick commented Oct 20, 2018

Uh oh!

Uh oh!