Add a SILGenCleanup pass and CanonicalizeInstruction utility. #24153

atrick · 2019-04-19T02:50:38Z

Canonicalize instruction will be a superset of
simplifyInstruction (once all the transforms are fixed for ownership
SIL). Additionally, it will also include simple SSA-based
canonicalization that requires new instruction creation. It may not
perform any optimization that interferes with diagnostics or increases
compile time.

The SILGenCleanup pass runs before diagnostics to perform any
canonicalization required by diagnostics.

Canonicalization replaces simplifyInstruction in SILCombine so we can
easily factor some existing SILCombine transforms into canonicalization.

gottesmm

Quick drive by. I want to do another pass through on this before I give the LGTM. Also can you split this up so that:

Adding the new code/utilities is one commit.
We have separate commits for each pass that are being updated.

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

lib/SILOptimizer/Analysis/SimplifyInstruction.cpp

lib/SILOptimizer/Mandatory/SILGenCleanup.cpp

gottesmm · 2019-04-22T21:38:07Z

lib/SILOptimizer/SILCombiner/SILCombine.cpp

@@ -113,6 +135,39 @@ void SILCombineWorklist::add(SILInstruction *I) {
  Worklist.push_back(I);
 }

+// Define a CanonicalizeInstruction subclass for use in SILCombine.
+class SILCombineCanonicalize : CanonicalizeInstruction {


Can you make this final/add override to all of the overridden entry points.

Yes, fixed.

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

atrick

I will rebase this soon after splitting it into 4 commits.

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

atrick · 2019-04-24T20:15:56Z

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

+  SILBasicBlock::iterator canonicalize(SILInstruction *inst);
+
+  /// Record a newly generated instruction.
+  virtual void genInstruction(SILInstruction *inst) = 0;


Much better, fixed.

lib/SILOptimizer/Mandatory/SILGenCleanup.cpp

atrick · 2019-04-24T20:36:33Z

lib/SILOptimizer/SILCombiner/SILCombine.cpp

@@ -113,6 +135,39 @@ void SILCombineWorklist::add(SILInstruction *I) {
  Worklist.push_back(I);
 }

+// Define a CanonicalizeInstruction subclass for use in SILCombine.
+class SILCombineCanonicalize : CanonicalizeInstruction {


Yes, fixed.

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

atrick · 2019-04-30T01:11:23Z

@gottesmm this PR now has the full set of commits that I want to get into 5.1 to fix exclusivity diagnostics. I'll make a separate PR for store canonicalization for master only.

atrick · 2019-04-30T01:11:32Z

@swift-ci test

atrick · 2019-04-30T01:11:44Z

@swift-ci test source compatibility

atrick · 2019-04-30T01:11:51Z

@swift-ci benchmark

swift-ci · 2019-04-30T01:18:08Z

Build failed
Swift Test OS X Platform
Git Sha - 6517180eec972c180bfe3b9af6c1d1d99eb33d5b

swift-ci · 2019-04-30T01:29:58Z

Build failed before running benchmark.

swift-ci · 2019-04-30T02:12:15Z

Build failed
Swift Test Linux Platform
Git Sha - 6517180eec972c180bfe3b9af6c1d1d99eb33d5b

atrick · 2019-05-01T07:22:45Z

@swift-ci test

atrick · 2019-05-01T07:22:55Z

@swift-ci benchmark

swift-ci · 2019-05-01T07:24:51Z

Build failed
Swift Test Linux Platform
Git Sha - 6517180eec972c180bfe3b9af6c1d1d99eb33d5b

swift-ci · 2019-05-01T07:26:32Z

Build failed
Swift Test OS X Platform
Git Sha - 6517180eec972c180bfe3b9af6c1d1d99eb33d5b

swift-ci · 2019-05-01T07:42:28Z

Build failed before running benchmark.

atrick · 2019-05-01T16:00:51Z

Forced pushed a minor test case fix for i386.

atrick · 2019-05-01T16:00:57Z

@swift-ci test.

atrick · 2019-05-01T16:01:04Z

@swift-ci benchmark.

swift-ci · 2019-05-01T16:01:18Z

Build failed
Swift Test Linux Platform
Git Sha - 9b201f0a616d7132066fc9b172ccaa8d995730e9

swift-ci · 2019-05-01T16:05:05Z

Build failed
Swift Test OS X Platform
Git Sha - 9b201f0a616d7132066fc9b172ccaa8d995730e9

atrick · 2019-05-01T16:16:28Z

@swift-ci benchmark

swift-ci · 2019-05-01T16:32:25Z

Build failed before running benchmark.

atrick · 2019-05-01T18:44:39Z

@swift-ci benchmark

atrick · 2019-05-01T18:46:32Z

@gottesmm I don't think there are any more build/test issues to fix here, but I'll wait to restart CI until you've finished your review.

swift-ci · 2019-05-01T19:05:34Z

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
ArrayPlusEqualArrayOfInt	260	280	+7.7%	0.93x (?)
ArrayAppendToFromGeneric	260	280	+7.7%	0.93x (?)
Improvement
DropLastArrayLazy	5	4	-20.0%	1.25x (?)

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
SortLettersInPlace	379	414	+9.2%	0.92x (?)

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

gottesmm

Some more suggestions. I went line by line. I need to read the load [copy] a bit more in depth. While you are looking at these I will look in more depth there.

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

lib/SILOptimizer/Mandatory/SILGenCleanup.cpp

include/swift/SIL/SILInstruction.h

…ers. This will make the forthcoming CanonicalizeInstruction interface more clear. This is generally the better approach to utilities that mutate the instruction stream. It avoids the temptation to assume that only a single instruction will be deleted or that only instructions before the current iterator will be deleted. This often happens to work but eventually fails in the presense of debug and end-of-scope instructions. A function returning an iterator has a more clear contract than one accepting some iterator reference of unknown providence. Unfortunately, it doesn't work at the lowest level of utilities, such as recursivelyDeleteTriviallyDeadInstructions, where we want to handle instruction batches.

CanonicalizeInstruction will be a superset of simplifyInstruction (once all the transforms are fixed for ownership SIL). Additionally, it will also include simple SSA-based canonicalization that requires new instruction creation. It may not perform any optimization that interferes with diagnostics or increases compile time. Canonicalization replaces simplifyInstruction in SILCombine so we can easily factor some existing SILCombine transforms into canonicalization.

The SILGenCleanup pass runs before diagnostics to perform any canonicalization required by diagnostics.

gottesmm

Some small additional asks. But beyond that... LGTM! This is a great PR! Great work Andy!

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

Reimplement load instruction canonicalization as part of the CanonicalizeInstruction utility.

Add -disable-diagnostic-passes because we want to test IRGen on precisely the SIL input sequence.

Canonical access markers should always guard some memory operation. It's valid to delete dead access markers even before exclusivity diagnostics. Deleting dead memory operations before diagnostics would *not* be ok.

This adds support to the load->struct_extract canonicalization for nontrivial element types which look like: load [copy] borrow struct_extract ...uses... end_borrow destroy

The recursivelyDeleteTriviallyDeadInstructions utility takes a callBack to be called for every deleted instruction. However, it wasn't passing this callBack to eraseFromParentWithdebugInsts. The callback was used to update an iterator in some cases, so not calling it resulted in iterator invalidation. Doing this also cleans up the both APIs: recursivelyDeleteTriviallyDeadInstructions and eraseFromParentWithdebugInsts.

atrick · 2019-05-07T00:26:56Z

@swift-ci test.

atrick · 2019-05-07T00:27:07Z

@swift-ci test source compatibility

swift-ci · 2019-05-07T00:28:54Z

Build failed
Swift Test OS X Platform
Git Sha - 3cda19a226722d3a5c5e127adce5a088b621b673

swift-ci · 2019-05-07T00:29:08Z

Build failed
Swift Test Linux Platform
Git Sha - 3cda19a226722d3a5c5e127adce5a088b621b673

[5.1] Merge pull request #24153 from atrick/fix-let-exclusivity

atrick requested a review from gottesmm April 19, 2019 02:50

gottesmm reviewed Apr 22, 2019

View reviewed changes

atrick commented Apr 24, 2019

View reviewed changes

gottesmm reviewed May 2, 2019

View reviewed changes

atrick commented May 2, 2019

View reviewed changes

include/swift/SIL/SILInstruction.h Show resolved Hide resolved

atrick added 3 commits May 6, 2019 08:36

Add a SILGenCleanup pass.

301ff8c

The SILGenCleanup pass runs before diagnostics to perform any canonicalization required by diagnostics.

gottesmm approved these changes May 6, 2019

View reviewed changes

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp Show resolved Hide resolved

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp Show resolved Hide resolved

atrick added 11 commits May 6, 2019 13:31

Canonicalize loads in SILGenCleanup.

f395f86

Reimplement load instruction canonicalization as part of the CanonicalizeInstruction utility.

SILGenCleanup tests.

e70b7c7

Fix IRGen/enum.sil RUN line.

4a11549

Add -disable-diagnostic-passes because we want to test IRGen on precisely the SIL input sequence.

Fixup access_markers.sil test case.

a27b96e

Canonical access markers should always guard some memory operation. It's valid to delete dead access markers even before exclusivity diagnostics. Deleting dead memory operations before diagnostics would *not* be ok.

Canonicalize nontrivial loads.

ca9ae0d

This adds support to the load->struct_extract canonicalization for nontrivial element types which look like: load [copy] borrow struct_extract ...uses... end_borrow destroy

Add a CanonicalizeInstruction test case for nontrivial loads.

c3c1dc7

Extend an exclusivity diagnostic test for nontrivial let's.

c3b7c19

Add a BeginBorrowInst::getEndBorrows helper.

9447d73

Add a comment on replaceUsesOfExtract per review feedback.

f151a96

Avoid else-return in splitAggregateLoad.

52237aa

atrick merged commit aa6347c into swiftlang:master May 7, 2019

atrick added a commit that referenced this pull request May 7, 2019

Merge pull request #24563 from atrick/5.1-fix-let-exclusivity

3007c4a

[5.1] Merge pull request #24153 from atrick/fix-let-exclusivity

atrick deleted the fix-let-exclusivity branch May 8, 2019 21:31

atrick mentioned this pull request May 7, 2019

[SR-10145] [Exclusivity] SILGen loads entire struct when reading captured 'let' stored property #52547

Open

Add a SILGenCleanup pass and CanonicalizeInstruction utility. #24153

Add a SILGenCleanup pass and CanonicalizeInstruction utility. #24153

Uh oh!

Conversation

atrick commented Apr 19, 2019

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!

Uh oh!

Uh oh!

gottesmm Apr 22, 2019

Choose a reason for hiding this comment

Uh oh!

atrick Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

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!

atrick Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atrick Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atrick commented Apr 30, 2019

Uh oh!

atrick commented Apr 30, 2019

Uh oh!

atrick commented Apr 30, 2019

Uh oh!

atrick commented Apr 30, 2019

Uh oh!

swift-ci commented Apr 30, 2019

Uh oh!

swift-ci commented Apr 30, 2019

Uh oh!

swift-ci commented Apr 30, 2019

Uh oh!

atrick commented May 1, 2019

Uh oh!

atrick commented May 1, 2019

Uh oh!

swift-ci commented May 1, 2019

Uh oh!

swift-ci commented May 1, 2019

Uh oh!

swift-ci commented May 1, 2019

Uh oh!

atrick commented May 1, 2019

Uh oh!

atrick commented May 1, 2019

Uh oh!

atrick commented May 1, 2019

Uh oh!

swift-ci commented May 1, 2019

Uh oh!

swift-ci commented May 1, 2019

Uh oh!

atrick commented May 1, 2019

Uh oh!

swift-ci commented May 1, 2019

Uh oh!

atrick commented May 1, 2019